Remove transport PSyAd code - #644
Conversation
… the kinds properly on scale factor calculations in the transport adjoint tests.
|
I am currently investigating an issue with one of the kernels that was passing during local canned test runs but suddenly started failing on both the local and test-suite... |
All better now, I think I must have deleted a line trying to remove white-space (the white-space did get removed at least). |
Tom Hill (tom-j-h)
left a comment
There was a problem hiding this comment.
Clearly all implemented correctly as tests are passing, and I've checked nothing has been missed (including removing everything that's no longer used). I haven't gone through all of the code in fine detail as the CR will but generally looks like style has been adhered to exactly.
Erica Neininger (ericaneininger)
left a comment
There was a problem hiding this comment.
Approved build changes, on behalf of SSD team
Mike Hobson (mike-hobson)
left a comment
There was a problem hiding this comment.
I can't see much wrong with the code in these changes. I just have a couple of very minor comments about using parameter statements.
When the kernels in this change were being generated by PSyAd, there was no practical way to unit test them, so the current testing system was introduced. This is great, and we should definitely keep those tests. However, now the kernels are being stored in the repository, they are now suitable for "normal" unit testing. Please add pFUnit-based unit tests for all the new kernels.
|
|
||
| ! Implied direction of outward normals dotted with basis functions. | ||
| ! If u*u_dot_n > 0 then this is the upwind cell | ||
| v_dot_n = (/ -1.0_r_tran, 1.0_r_tran, 1.0_r_tran, -1.0_r_tran /) |
There was a problem hiding this comment.
v_dot_n appears to be a constant, so it should be declared as a parameter.
| real(kind=r_def), dimension(nfaces) :: v_dot_n | ||
|
|
||
| ! Implied direction of outward normals dotted with basis functions. | ||
| ! If u*u_dot_n > 0 then this is the upwind cell | ||
| v_dot_n = (/ -1.0_r_def, 1.0_r_def, 1.0_r_def, -1.0_r_def /) |
There was a problem hiding this comment.
Again, v_dot_n appears to be a constant, so it should be declared as a parameter.
Thanks Mike. The current testing system was introduced because the test that really matters for the adjoint is that it is the matrix transpose of the tangent linear/forward. Even if we did things using source code all throughout we would have still needed the Happy to do the other changes as soon as possible! |
I am completely happy that your current testing system really matters and I am definitely not asking you to remove it. Within the LFRic code bases we require kernels to be unit tested, unless you have a very good reason why they cannot be. I'm guessing that your collective agreement to not have unit tests was made because the kernels were generated (and that would be a good reason not to unit test them). Now they are not generated and I can't see a reason not to unit test them. If you can give me a really good reason why you can't unit test these kernels I'll happily drop the request. Otherwise, please provide unit tests. |
PR Summary
Sci/Tech Reviewer: Tom Hill (@tom-j-h)
Code Reviewer: Mike Hobson (@mike-hobson)
Removed the PSyAd patches for the transport adjoints. Unstaged the algebra kernels and test algorithms from being built via PSyAd. Added the transport adjoint kernels and test algorithms as source code in adjoint and adjoint_tests respectively. I have also taken the liberty to make them faster to run by simplifying a lot of the PSyAdisms.
N.B.: Unit testing adjoint kernels is a bit different to usual kernels. There are no pFUnit tests for these kernels because the unit test for them involves testing against the tangent linear source code (i.e.: testing against kernels/algorithms in lfric_core, gungho, linear, etc.). These have been committed onto the branch.
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - wololo_psyad_transport/run5
Suite Information
Task Information
✅ succeeded tasks - 1218
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