Skip to content
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

(Closes 2311) Add functionality to derive module and subroutine names of adjoint test from filename for LFRic API #2749

Merged
merged 26 commits into from
Nov 4, 2024

Conversation

DrTVockerodtMO
Copy link
Collaborator

@DrTVockerodtMO DrTVockerodtMO commented Oct 21, 2024

Closes #2311 which has a description of the problem.

Patch notes

I solved the problem by feeding the filename given to -otest down into the codebase. Here is a summary of the changes made across the relevant call tree:

src/psyclone/psyad/main.py
Changed the default value of this variable to be None. Added in the args.test_filename value to the call signature of generate_adjoint_str.

src/psyclone/psyad/tl2ad.py
Added new test_filename argument into call signature. I decided to add the conversion of the test_filename into the test_name variable here because I figured it would be API specific, so having it in the API selection telegraphs that nicely. Default value is just adjoint_test like the previous behaviour.

For the LFRic API, we take everything from test_filename before "_mod." to be the test_name.

src/psyclone/psyad/domain/lfric/lfric_adjoint_harness.py
Changed function to use test_name instead of the hardcoded value of "adjoint_test".

Testing

I tested this by making changes to src/psyclone/tests/psyad/domain/lfric/test_lfric_adjoint_harness.py. I passed in a new value of the optional test_name that is "adjoint_test_alg" which would replicate what we see in LFRic. This test passed by producing the expected "adjoint_test_alg_mod" as the module name and "adjoint_test_alg" as the subroutine name.

I also tested this by changing src/psyclone/tests/psyad/tl2ad_test.py to include a test_filename KGO, which also passed.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (8367de1) to head (567148e).
Report is 27 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2749   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         354      354           
  Lines       49010    49021   +11     
=======================================
+ Hits        48946    48957   +11     
  Misses         64       64           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for tackling this Terry. I think the change is generally good but I'm requesting a bit more validation as we now care about the form of the filename (for the LFRic API) whereas previously we didn't.
Ref. guide is up to 18 warnings but that's not the fault of this PR.
There are no indirect coverage changes.
I'll run the integration tests next time around as I don't forsee any problems.

src/psyclone/psyad/domain/lfric/lfric_adjoint_harness.py Outdated Show resolved Hide resolved
src/psyclone/psyad/tl2ad.py Outdated Show resolved Hide resolved
src/psyclone/psyad/tl2ad.py Outdated Show resolved Hide resolved
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good now Terry, thanks for those changes.
As is often the case, looking at the new test made me realise that we could improve the code itself slightly by performing the filename validation a bit earlier. I also need some justification for the first new test you've added.
Back to you! (You'll need to bring your branch up to master as a couple of other PRs have now been merged.)

examples/psyad/eg2/README.md Show resolved Hide resolved
examples/psyad/eg2/README.md Outdated Show resolved Hide resolved
src/psyclone/psyad/main.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyad/main_test.py Outdated Show resolved Hide resolved
@DrTVockerodtMO DrTVockerodtMO mentioned this pull request Oct 30, 2024
11 tasks
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DrTVockerodtMO, I think just a little bit more tidying/refactoring (to bring the validation slightly earlier) and then this is ready to go.
I'll fire off the integration tests now.

src/psyclone/psyad/main.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyad/main_test.py Outdated Show resolved Hide resolved
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Terry. Just a final bit of tidying/simplifying of the associated test and then this is good to go.

src/psyclone/tests/psyad/main_test.py Outdated Show resolved Hide resolved
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining requested change has been made, thanks :-)
Will proceed to update, run CI and merge.

@arporter arporter merged commit 65e3232 into master Nov 4, 2024
13 checks passed
@arporter arporter deleted the 2311_adjoint_test_naming branch November 4, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LFRic] PSyAD generated adjoint tests do not update module and subroutine names correctly
2 participants