-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: master
Are you sure you want to change the base?
Conversation
…ubroutine and module names
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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 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.
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.
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.)
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 @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
@@ -139,13 +142,30 @@ def msg(): | |||
logger.error("psyad error: file '%s', not found.", filename) | |||
sys.exit(1) | |||
|
|||
# Processing filename |
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.
If you move this check up to L133 we can simplify the associated test as it will no longer need valid filenames for anything.
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.
Addressed in b212e07.
def test_main_otest_lfric_error_name(tmpdir, capsys, caplog): | ||
''' Test that a bad -otest option combined with LFRic API | ||
generates the expected error. ''' | ||
filename_in = str(tmpdir.join("tl_foo_kernel_mod.f90")) |
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.
As commented earlier, with a slight re-ordering of the validation you should be able to run this test without any valid filenames/files.
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.
Addressed in b212e07.
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 theargs.test_filename
value to the call signature ofgenerate_adjoint_str
.src/psyclone/psyad/tl2ad.py
Added new
test_filename
argument into call signature. I decided to add the conversion of thetest_filename
into thetest_name
variable here because I figured it would be API specific, so having it in the API selection telegraphs that nicely. Default value is justadjoint_test
like the previous behaviour.For the LFRic API, we take everything from
test_filename
before "_mod." to be thetest_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 optionaltest_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 atest_filename
KGO, which also passed.