-
Notifications
You must be signed in to change notification settings - Fork 33
(closes #3216) Allow specifying explicit private variables for parallel regions #3275
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
base: master
Are you sure you want to change the base?
Conversation
|
@MetBenjaminWent may want to be aware of /test these changes |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3275 +/- ##
=======================================
Coverage 99.95% 99.95%
=======================================
Files 376 376
Lines 53485 53504 +19
=======================================
+ Hits 53463 53482 +19
Misses 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@arporter @LonelyCat124 This is ready for a first look |
|
I'm not going to review this today, just having a quick look at it now - is it worth having an example showing how to use this? We had people in NG-Arch using explicit private on |
|
When merging the NG-Arch tickets its use has been standarised to the "mark_explicit_privates" from the LFRic psyclone_tools.py that I show in my top comment, so it is a one-line change. But I agree regarding the example/more docs comment. |
|
I added and example as suggested (currently in nemo/eg8, but we could move it to the psyir folder once there is no conflict with @LonelyCat124 current changes). The documentation is now part of the Transformation options. This is done on purpose to make it more discoverable, it is what people will probably open. And it is automatically inherited to all subclasses through kwargs: @LonelyCat124 @arporter This is ready to review again |
LonelyCat124
left a comment
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.
@sergisiso This mostly looks ok to me, I've not done all the local stuff yet - couple of copyright dates need updating but I'll do a final review once we can run integration with a local lfric_atm/core
| # ----------------------------------------------------------------------------- | ||
| # BSD 3-Clause License | ||
| # | ||
| # Copyright (c) 2020-2026, Science and Technology Facilities Council. |
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.
Copyright date should just be 2026 right? Since its a new file
| # ----------------------------------------------------------------------------- | ||
| # BSD 3-Clause License | ||
| # | ||
| # Copyright (c) 2018-2025, Science and Technology Facilities Council |
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.
Copyright year.
| self._check_variable(var) | ||
| self._variable = var | ||
|
|
||
| def replace_symbols_using(self, table_or_symbol): |
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.
Can you update this to use type hints instead of the doc :type:?
| ''' | ||
|
|
||
| @staticmethod | ||
| def _attempt_privatisation(loop, symbol_name, dry_run=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.
What happened to the dry_run option here? I guess its not needed because we no longer modify the tree, only affect some local variables to the transformation and resultant directive>

Currently we have
Loop.explictly_private_symbolsto force a different result than the automatic sharing attributes provide. However it is not possible to do the same with parallel regions. To fix this I moved this attribute to the SharingAttributeMixin. It makes more sense here as it is being set and used by the same class (better encapsulation) and can be used by ALL subclases adding sharing attribute clauses.There was also the problem that users didn't find out this capability easily. This can be improved by adding a transformation option, as this is the documentation that people is looking at while transforming code.
So when LFRic_atm scripts currently do:
can be:
The transformation option logs (but doesn't fail) for symbols not found, to have the same behaviour as
mark_explicit_privates.