Update docs with changes to Transmute#261
Update docs with changes to Transmute#261MetBenjaminWent wants to merge 6 commits intoMetOffice:mainfrom
Conversation
oakleybrunt
left a comment
There was a problem hiding this comment.
Thanks Ben, I think this is really good information that is important to record. I have suggested some tweaks to the wording in places to improve calrity, though may have missed some key information. Feel free to add/take away from my suggestions as you see fit.
documentation/source/developer_guide/psyclone/psyclone_makefiles.rst
Outdated
Show resolved
Hide resolved
documentation/source/developer_guide/psyclone/psyclone_makefiles.rst
Outdated
Show resolved
Hide resolved
documentation/source/developer_guide/psyclone/psyclone_scripts.rst
Outdated
Show resolved
Hide resolved
documentation/source/developer_guide/psyclone/psyclone_scripts.rst
Outdated
Show resolved
Hide resolved
| ``PSYCLONE_PASS_NO_SCRIPT`` has been implemented primarily for the ``TRANSMUTE_INCLUDE_METHOD`` | ||
| ``specify_include``, however can be expanded to either. | ||
| It controls whether files are to be passed to PSyclone, but not to have a script applied to them. | ||
| This can be useful to remove existing clauses from a file, but not add anything back. | ||
| files to be affected this way should only be added to this list, as they will be filtered out of | ||
| ``PSYCLONE_PHYSICS_FILES`` anyway. |
There was a problem hiding this comment.
| ``PSYCLONE_PASS_NO_SCRIPT`` has been implemented primarily for the ``TRANSMUTE_INCLUDE_METHOD`` | |
| ``specify_include``, however can be expanded to either. | |
| It controls whether files are to be passed to PSyclone, but not to have a script applied to them. | |
| This can be useful to remove existing clauses from a file, but not add anything back. | |
| files to be affected this way should only be added to this list, as they will be filtered out of | |
| ``PSYCLONE_PHYSICS_FILES`` anyway. | |
| ``PSYCLONE_PASS_NO_SCRIPT`` specifies files to be passed to PSyclone without any associated transformation scripts. | |
| The primary use case is to remove existing clauses from a file without modifying the Fortran source. | |
| Any files included in ``PSYCLONE_PASS_NO_SCRIPT`` are filtered out of the ``PSYCLONE_PHYSICS_FILES`` list. | |
| No transformation scripts will be applied to files in this list regardless of the ``TRANSMUTE_INCLUDE_METHOD``. |
There was a problem hiding this comment.
Currently I'm not sure specify_exclude actually works for PSYCLONE_PASS_NO_SCRIPT.
I've implemented a very minimum viable product currently, whilst FAB/BAF or simular is being developed. I think I wanted to highlight this here somewhat that the intended use is with specify_include at this time.
There was a problem hiding this comment.
Thanks, I wasn't aware of that. Can I suggest modifying the wording to reflect this? Currently, the phrase "can be expanded to either" suggests to me that users can use PSYCLONE_PASS_NO_SCRIPT with specify_exclude. Maybe this is clearer?
| ``PSYCLONE_PASS_NO_SCRIPT`` has been implemented primarily for the ``TRANSMUTE_INCLUDE_METHOD`` | |
| ``specify_include``, however can be expanded to either. | |
| It controls whether files are to be passed to PSyclone, but not to have a script applied to them. | |
| This can be useful to remove existing clauses from a file, but not add anything back. | |
| files to be affected this way should only be added to this list, as they will be filtered out of | |
| ``PSYCLONE_PHYSICS_FILES`` anyway. | |
| ``PSYCLONE_PASS_NO_SCRIPT`` has been implemented for the ``TRANSMUTE_INCLUDE_METHOD`` | |
| ``specify_include``. Use of this variable for the ``specify_exclude`` method is not mature, | |
| though work is planned (see [GitHub issue number here]). | |
| This variable specifies files to be passed to PSyclone without applying any transformations. | |
| This can be useful to remove existing clauses from a file without modifying the Fortran source. | |
| Any files included in ``PSYCLONE_PASS_NO_SCRIPT`` are filtered out of the ``PSYCLONE_PHYSICS_FILES`` list. |
There was a problem hiding this comment.
-
I agree that the wording would be a little clearer if it were changed to something like Oakley's suggestions for lines 128 and 130 specifically:
- "This variable specifies files to be passed to PSyclone without applying any transformations.
and - "Any files included in
PSYCLONE_PASS_NO_SCRIPTare filtered out of thePSYCLONE_PHYSICS_FILESlist." .
And then you could leave in your other lines to just reflect what is currently possible?
- "This variable specifies files to be passed to PSyclone without applying any transformations.
-
Maybe you could also change line 126 to "however this could be expanded to exclude in the future" ?
documentation/source/developer_guide/psyclone/psyclone_scripts.rst
Outdated
Show resolved
Hide resolved
…es.rst Extend advice for global variable Co-authored-by: Oakley Brunt <[email protected]>
Add suggestions from SR Co-authored-by: Oakley Brunt <[email protected]>
|
@oakleybrunt, thanks for the SR, I've implemented the changes which are viable, though I've not accepted one as the trunk functionality is a little underbaked currently (Given FAB coming and requests from Chris to keep my implementations need only), I wanted the docs to reflect what is currently possible. |
mo-lucy-gordon
left a comment
There was a problem hiding this comment.
Overall this looks good but I also left a comment about suggested wording changes
PR Summary
Linked issue: #25
A few issues and PRs to apps and core have adjusted slightly how transmute functions.
To improve the docs and bring them in line with its current usage with the make build system, we need to update a few sections.
See #49, #246 and MetOffice/lfric_core#271
Sci/Tech Reviewer: @oakleybrunt
Code Reviewer: @mo-lucy-gordon
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - update_transmute_docs/run1
Suite Information
Task Information
❌ failed tasks - 3
⌛ waiting tasks - 4
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review