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. |
Thank you for the response. Re-reading my comment I clearly didn't go into enough detail, my apologies. I have written a small document regarding the reasoning why we (in my opinion) ought to not do unit testing of the kernels that I will send your way ASAP. |
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Code owner (build system) review: Removing items from existing lists in line with current usage.
|
Thanks for the document. As I'm responding to items from it, I've attached it, here. I think there may be some misunderstanding about the purpose of unit testing. A number of the reasons you give for not adding unit tests are actually core qualities of this form of testing. For example, unit tests should be "brittle" (there's no point in having a robust unit test that will never fail) and unit tests should always be redundant (if the only place we test some code is a unit test, then we are definitely missing multiple layers of testing). Whilst test-driven development can be a great way to develop, I accept that here, the process is to create kernels that are the transpose of the linear kernels, not simply kernels that pass a pre-written unit test. However, there are more benefits to unit testing than simply supporting test-driven development. Unit tests are not tests of the science. They should not be used, for example, to test that an adjoint is the transpose of the linear code. The unit test should have no knowledge of the purpose of the kernel. The code being called should take a minimal set of fixed inputs. The inputs don't even have to be physically sensible (they often are as this is usually easier). The code is exercised using those inputs and the results are checked to make sure the code has behaved as expected. There should be no science here - it is an almost mechanical test. Bear in mind that even if you don't think unit tests are useful to you, they will be useful to others. For example, a member of the HPC Optimisation team may come across the kernel in a few year's time and think they might have an optimisation. The unit test is the ideal test to run early in the development process to make sure they haven't broken the kernel. I don't think any of your arguments are the "smoking gun" that would allow me to ignore our coding standards and commit these changes without unit tests, so I still think you need to add unit tests. For further information, please see the notes on unit testing in our documentation: https://metoffice.github.io/lfric_core/how_to_use_it/testing/unit_testing.html |
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