-
Notifications
You must be signed in to change notification settings - Fork 64
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
Handle dimensions for promoted variables #562
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.
Couple of questions about code
scripts/ccpp_suite.py
Outdated
else: | ||
# Add dimensions if they're not already there | ||
group.add_variable_dimensions(var, [], | ||
adjust_intent=True, | ||
to_dict=group.call_list) | ||
# end if | ||
# end if | ||
# end for |
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 a case for this? Is it tested (capgen test seems to run fine without it)?
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 case would be a promoted variable with no horizontal dimension, but the existing dimension(s) do still need to be added to the calling list for the init routine.
That said, thanks to your other suggestion, I reworked the code so this "else" is no more. I also altered the capgen_test to ensure promotion/allocation works as expected for a variable with no horizontal dimension
scripts/suite_objects.py
Outdated
else: | ||
dims = [new_horiz_dim] |
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 the only other possibility? No other dimensions allowed?
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.
A very good question.
Updated the logic in ccpp_suite to replace the horizontal dimension where it is and not remove other dimensions! This logic is now gone though, as I discovered it was unnecessary (the logic in ccpp_suite to promote variables handles the dimensions)
@peverwhee Maybe (probably) I don't understand the promotion business in Capgen, but looking at the test code I'm confused on the spirit of these changes. |
To understand why this works, you have to know / learn about another corner of capgen. From the point of view of I hope this helps and does not just make everything more confusing. |
@gold2718 Thanks for the explanation. Makes sense. |
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 good now, 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.
Looks good to me, and I think I actually understand what it is doing ;-)
Merged into #549 |
Description
Handle edge case where variable dimensions weren't getting added to the init phase calling list for allocation of promoted, suite-level variables.
Also includes logic to use horizontal_dimension in allocate statement for run-time only variables (with dimension horizontal_loop_extent)
Changes
User interface changes?: No
Fixes
closes #559
Testing
Test added for runtime-only promoted variables