Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DeadDataFlowElimination will add type hint when removing a connector #1499

Merged
merged 12 commits into from
Feb 23, 2024
Merged

DeadDataFlowElimination will add type hint when removing a connector #1499

merged 12 commits into from
Feb 23, 2024

Conversation

luca-patrignani
Copy link
Contributor

Issue #1150 that DeadDataflowElimination removes a connector from a Tasklet which leaves a variable without type hint.
This PR tries to fix this bug by adding a type hint expression for a variable which is used in the tasklet. It adds the type hint only if the variable is used inside the tasklet code (I checked using ASTFindReplace).
The PR also adds a test which is literaly the code presented in #1150 and asserts the presence of the type hint and checks if it compiles.

May need confirmation

  • Did I use ASTFindReplace correctly?
  • If the type inference fails no type hint is added. Is it the right solution?
  • Does the test even make sense? (I don't have much experience in unit testing).

This is my first PR for this project so be patient with me.

@luca-patrignani
Copy link
Contributor Author

@tbennun can you please approve the workflows?

@tbennun
Copy link
Collaborator

tbennun commented Jan 12, 2024

Thank you for the PR! Unfortunately our CI is currently undergoing fixing so failing tests may not mean that your PR broke them.

@tbennun
Copy link
Collaborator

tbennun commented Jan 12, 2024

@luca-patrignani as for your questions:

  1. It is a bit of a nonstandard use for ASTFindReplace, but should be OK. Please add comments annotating what the try/except block means so that future developers can easily see that you do not intend to replace anything.
  2. If done correctly, type inference should always pass, and if not the code generator will use the auto keyword (which was done before this PR and not always correct).
  3. The test looks good. If you're already compiling the SDFG, I would also run it with random numpy arrays and verify that it produces the correct output (np.where will produce the same answer in the Python world).

@luca-patrignani
Copy link
Contributor Author

@tbennun can you please approve the workflows once again?

@luca-patrignani
Copy link
Contributor Author

@tbennun when you have some free time can we continue this PR review?

@tbennun
Copy link
Collaborator

tbennun commented Feb 19, 2024

@luca-patrignani please stop merging the master branch to this one. When merging the PR, we will squash and merge the updated version as necessary.

Copy link
Collaborator

@tbennun tbennun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you again for contributing this fix!

@tbennun tbennun added the in the merge queue waiting for CI to work again label Feb 19, 2024
@alexnick83 alexnick83 added this pull request to the merge queue Feb 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 21, 2024
@alexnick83 alexnick83 added this pull request to the merge queue Feb 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 21, 2024
@alexnick83 alexnick83 added this pull request to the merge queue Feb 21, 2024
@phschaad phschaad removed this pull request from the merge queue due to the queue being cleared Feb 22, 2024
@alexnick83 alexnick83 added this pull request to the merge queue Feb 22, 2024
Merged via the queue into spcl:master with commit bc08e9a Feb 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in the merge queue waiting for CI to work again
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants