-
Notifications
You must be signed in to change notification settings - Fork 12
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
assumed size array handling for 'normalize_array_shape_and_access' #218
assumed size array handling for 'normalize_array_shape_and_access' #218
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/218/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
=======================================
Coverage 92.26% 92.26%
=======================================
Files 96 96
Lines 17124 17135 +11
=======================================
+ Hits 15799 15810 +11
Misses 1325 1325
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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, looks good. Two suggestions where I think stuff could be simplified, otherwise GTG!
return (isinstance(dim, sym.RangeIndex) and not dim.lower == 1 and not dim is None | ||
and not dim.lower is None and not dim.upper is None) |
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.
This expression looks odd, I think it could be simplified:
return (isinstance(dim, sym.RangeIndex) and not dim.lower == 1 and not dim is None | |
and not dim.lower is None and not dim.upper is None) | |
return isinstance(dim, sym.RangeIndex) and not (dim.lower == 1 or dim.lower is None or dim.upper is None) |
|
||
vmap = {} | ||
for v in FindVariables(unique=False).visit(routine.body): | ||
if isinstance(v, sym.Array): | ||
# skip if e.g., `array(len)`, passed as `call routine(array)` | ||
if len(v.shape) != len(v.dimensions): |
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.
Shouldn't the only case be that v.dimensions is None
? Otherwise, shape
and dimensions
should match or there's something fishy anyway.
|
||
def validate_routine(routine): | ||
arrays = [var for var in FindVariables().visit(routine.body) if isinstance(var, Array)] | ||
for arr in arrays: | ||
# print(f"arr: {arr} | {all(not isinstance(shape, sym.RangeIndex) for shape in arr.shape)}") |
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.
# print(f"arr: {arr} | {all(not isinstance(shape, sym.RangeIndex) for shape in arr.shape)}") |
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 making these changes, looks good now!
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.
Looks great, going in now.
normalize_array_shape_and_access()
didn't handle assumed size arrays correctly.This is a proposed fix/extension.