-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fix check for redundant loop priority #895
Conversation
Co-authored-by: Connor Ward <c.ward20@imperial.ac.uk>
8e2656d
to
7d4adfb
Compare
@@ -862,10 +863,14 @@ def _update_nesting_constraints( | |||
.ancestors(inner_iname_nest)) | |||
ancestors_of_outer_iname = (loop_nest_tree | |||
.ancestors(outer_iname_nest)) | |||
if outer_iname in ancestors_of_inner_iname: |
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.
Would have been nice if mypy
could tell us our comparison types are incompatible. We were checking "InameStr
in tuple[InameStrSet, ...]
" which would always return False.
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 agree. But __contains__
is typed for object
: https://github.com/python/typeshed/blob/d70fad09dd7012bd43304e1c67b7599558b90c39/stdlib/typing.pyi#L575
😢
To their credit, the Python types people have discussed it: python/typeshed#8219
Thanks! |
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.
Thanks for working on this! A minor nit below, otherwise LGTM!
@@ -862,10 +863,14 @@ def _update_nesting_constraints( | |||
.ancestors(inner_iname_nest)) | |||
ancestors_of_outer_iname = (loop_nest_tree | |||
.ancestors(outer_iname_nest)) | |||
if outer_iname in ancestors_of_inner_iname: |
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 agree. But __contains__
is typed for object
: https://github.com/python/typeshed/blob/d70fad09dd7012bd43304e1c67b7599558b90c39/stdlib/typing.pyi#L575
😢
To their credit, the Python types people have discussed it: python/typeshed#8219
FYI: Firedrake failures are expected for the time being, see #894. |
7d4adfb
to
6151bd3
Compare
6151bd3
to
ffe77c3
Compare
Closes #890.
The attached regression fails on
main
./cc @connorjward