PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

(Closes #1496) Updated branch adding PSyAD test harness support

Open arporter opened this issue 3 years ago • 2 comments

arporter avatar Jul 01 '22 07:07 arporter

Codecov Report

Base: 99.51% // Head: 99.52% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (c556836) compared to base (2f8f5bf). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1782    +/-   ##
========================================
  Coverage   99.51%   99.52%            
========================================
  Files         272      273     +1     
  Lines       39768    39943   +175     
========================================
+ Hits        39576    39752   +176     
+ Misses        192      191     -1     
Impacted Files Coverage Δ
src/psyclone/domain/lfric/algorithm/__init__.py 100.00% <ø> (ø)
src/psyclone/psyad/domain/common/__init__.py 100.00% <ø> (ø)
src/psyclone/domain/lfric/algorithm/lfric_alg.py 100.00% <100.00%> (ø)
...psyclone/domain/lfric/kern_call_invoke_arg_list.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/psyir.py 100.00% <100.00%> (ø)
src/psyclone/psyad/domain/common/adjoint_utils.py 100.00% <100.00%> (ø)
src/psyclone/psyad/domain/lfric/__init__.py 100.00% <100.00%> (ø)
...yad/domain/lfric/generate_lfric_adjoint_harness.py 100.00% <100.00%> (ø)
src/psyclone/psyad/main.py 100.00% <100.00%> (ø)
src/psyclone/psyad/tl2ad.py 100.00% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Jul 01 '22 11:07 codecov[bot]

I tried merging in master and fixing the conflicts and it got quite confusing. I'm therefore going to break-out the ability to specify the API from this PR into a separate one.

arporter avatar Aug 12 '22 08:08 arporter

I think this is pretty much there now. I need to double-check the generated code inside an LFRic mini-app.

arporter avatar Sep 09 '22 08:09 arporter

I've found two problems:

  1. the LFRic build system has PSyclone's line-length limiting turned on for input files and this rejects the files produced by PSyAD;
  2. the generated test harness is missing a declaration for function_space_collection.

I've fixed 1. for now by turning on line-length limiting for all output from PSyAD. #1863 will make this a command-line option. I've had the test harness building and working previously so I must have lost the code that declares function_space_collection during a merge.

arporter avatar Sep 09 '22 09:09 arporter

I couldn't work out where/when the declaration disappeared - it's probably when I gave up on the idea of creating a standalone program and went for an LFRic mini-app instead. I've added the generation of the missing symbol and now:

20220909111117.218+0100:INFO : skeleton: Miniapp initialised
 Test of adjoint of 'tl_rhs_sample_eos_kernel_type' PASSED:    3.0664860371160282E+017   3.0664860371161082E+017   125.00000000000000     
20220909111117.240+0100:S1:INFO : skeleton: Writing diagnostic output

I just need to document what has to be done to the skeleton mini-app to get this working.

arporter avatar Sep 09 '22 10:09 arporter

I just need to fix a few more pylint issues and check docstrings are OK now.

arporter avatar Sep 09 '22 13:09 arporter

I'm afraid this is still quite a biggie but it's very pleasing :-) Ready for review from @rupertford or perhaps @hiker.

arporter avatar Sep 09 '22 15:09 arporter

One thing I found slightly confusing was the use of _test in for the harness as we obviously use that for test code as well. It might be better if _test were replaced with _harness.

I've had a go at renaming things. Hopefully it's better now.

arporter avatar Sep 26 '22 09:09 arporter

Ready for review again now.

arporter avatar Sep 26 '22 16:09 arporter

Ready for another look, assuming CI happiness.

arporter avatar Oct 04 '22 21:10 arporter