-
Notifications
You must be signed in to change notification settings - Fork 48
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
Propagate foast & past typing information to ITIR #1199
Propagate foast & past typing information to ITIR #1199
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good. Just a couple some questions, no need to change anything.
) | ||
inferred = ti.infer(testee) | ||
assert inferred == expected | ||
assert ti.pformat(inferred) == "(It[T₁, T₂, float⁰]) → It[T₁, T₂, float⁰]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is being tested here that hasn't been in the first assert
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea of @fthaler was to test the type pretty printer like this. Additionally this gives a more readable representation of the inferred type.
if node.kind: | ||
kind = {"Iterator": Iterator(), "Value": Value()}[node.kind] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not very important, but I think it would better to use explicit pattern matching:
if node.kind: | |
kind = {"Iterator": Iterator(), "Value": Value()}[node.kind] | |
match node.kind: | |
case "Iterator": | |
kind = Iterator() | |
case "Value": | |
kind = Value() | |
case _: | |
raise AssertionError(...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the point, but I like the less verbose version more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, for just two cases, I don't think the dict version brings any value. Even a simple if-else
would be better.
@tehrengruber What is the current state of this PR? Is it blocked by something else? |
@egparedes you never clicked approve on this ;) we're waiting for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me.
Adds support for constraining the type of symbols in ITIR (only
dtype
andkind
right now) and propagates the information from FOAST and PAST to ITIR.