-
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
SCCBaseTransformation.get_integer_variable
now also checks module imports
#279
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/279/index.html |
45058ec
to
3438c41
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
- Coverage 92.88% 92.88% -0.01%
==========================================
Files 102 102
Lines 18253 18252 -1
==========================================
- Hits 16954 16953 -1
Misses 1299 1299
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, nice find! A question on the test and a suggestion how to maybe save a few cycles in the transformation.
@@ -796,6 +804,9 @@ def test_single_column_coalesced_hoist_openacc(frontend, horizontal, vertical, b | |||
analysis = HoistTemporaryArraysAnalysis(dim_vars=(vertical.size,)) | |||
synthesis = SCCHoistTemporaryArraysTransformation(block_dim=blocking) | |||
|
|||
# Check that blocking size has not been redefined | |||
assert driver.symbol_map[blocking.size].type.module |
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.
Is this missing some kind of comparison maybe? E.g., driver.symbol_map[blocking.size].type.module.lower() == 'block_dim_mod'
?
if name in routine.symbol_map: | ||
v_index = routine.symbol_map[name] | ||
else: |
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.
Since constructing these maps is expensive, could we maybe do it like this?
if name in routine.symbol_map: | |
v_index = routine.symbol_map[name] | |
else: | |
v_index = routine.symbol_map.get(name) | |
if v_index is None: |
3438c41
to
b6fc44a
Compare
b6fc44a
to
47314ba
Compare
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.
Many thanks, I think this is uncontroversial and good to go!
Confirmed clean with ec-physics. |
The
get_integer_variable
utility inSCCBaseTransformation
now also checks if the integer has been imported via a module.