Skip to content

Remove transport PSyAd code - #644

Open
DrTVockerodtMO wants to merge 19 commits into
MetOffice:mainfrom
DrTVockerodtMO:convert_psyad_transport
Open

Remove transport PSyAd code#644
DrTVockerodtMO wants to merge 19 commits into
MetOffice:mainfrom
DrTVockerodtMO:convert_psyad_transport

Conversation

@DrTVockerodtMO

@DrTVockerodtMO DrTVockerodtMO commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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

  • I have performed a self-review of my own code
  • My code follows the project's style guidelines
  • Comments have been included that aid understanding and enhance the readability of the code
  • My changes generate no new warnings
  • All automated checks in the CI pipeline have completed successfully

Testing

  • I have tested this change locally, using the LFRic Apps rose-stem suite
  • If any tests fail (rose-stem or CI) the reason is understood and acceptable (e.g. kgo changes)
  • I have added tests to cover new functionality as appropriate (e.g. system tests, unit tests, etc.)
  • Any new tests have been assigned an appropriate amount of compute resource and have been allocated to an appropriate testing group (i.e. the developer tests are for jobs which use a small amount of compute resource and complete in a matter of minutes)

trac.log

Test Suite Results - lfric_apps - wololo_psyad_transport/run5

Suite Information

Item Value
Suite Name wololo_psyad_transport/run5
Suite User terence.vockerodt
Workflow Start 2026-07-21T15:03:42
Groups Run developer
Dependency Reference Main Like
casim MetOffice/casim@2026.07.1 True
jules MetOffice/jules@2026.07.1 True
lfric_apps DrTVockerodtMO/lfric_apps@convert_psyad_transport False
lfric_core MetOffice/lfric_core@2026.07.1 True
moci MetOffice/moci@2026.07.1 True
SimSys_Scripts MetOffice/SimSys_Scripts@2026.07.1 True
socrates MetOffice/socrates@2026.07.1 True
socrates-spectral MetOffice/socrates-spectral@2026.07.1 True
ukca MetOffice/ukca@2026.07.1 True

Task Information

✅ succeeded tasks - 1218

Security Considerations

  • I have reviewed my changes for potential security issues
  • Sensitive data is properly handled (if applicable)
  • Authentication and authorisation are properly implemented (if applicable)

Performance Impact

  • Performance of the code has been considered and, if applicable, suitable performance measurements have been conducted

AI Assistance and Attribution

  • Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy (including attribution labels)

Documentation

  • Where appropriate I have updated documentation related to this change and confirmed that it builds correctly

PSyclone Approval

  • If you have edited any PSyclone-related code (e.g. PSyKAl-lite, Kernel interface, optimisation scripts, LFRic data structure code) then please contact the TCD Team

Sci/Tech Review

  • I understand this area of code and the changes being added
  • The proposed changes correspond to the pull request description
  • Documentation is sufficient (do documentation papers need updating)
  • Sufficient testing has been completed

(Please alert the code reviewer via a tag when you have approved the SR)

Code Review

  • All dependencies have been resolved
  • Related Issues have been properly linked and addressed
  • CLA compliance has been confirmed
  • Code quality standards have been met
  • Tests are adequate and have passed
  • Documentation is complete and accurate
  • Security considerations have been addressed
  • Performance impact is acceptable

@DrTVockerodtMO DrTVockerodtMO self-assigned this Jul 21, 2026
@DrTVockerodtMO DrTVockerodtMO added this to the Autumn 2026 milestone Jul 21, 2026
@DrTVockerodtMO DrTVockerodtMO changed the title Remove PSyAd transport code Remove transport PSyAd code Jul 21, 2026
@DrTVockerodtMO

Copy link
Copy Markdown
Contributor Author

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...

@DrTVockerodtMO

Copy link
Copy Markdown
Contributor Author

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-j-h Tom Hill (tom-j-h) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved build changes, on behalf of SSD team

@mike-hobson Mike Hobson (mike-hobson) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 /)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

v_dot_n appears to be a constant, so it should be declared as a parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 346ecb9.

Comment on lines +95 to +99
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 /)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, v_dot_n appears to be a constant, so it should be declared as a parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 346ecb9.

@DrTVockerodtMO

Copy link
Copy Markdown
Contributor Author

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.

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 adjoint_tests app present. We collectively agreed early on that this would be the way to test the adjoint and that unit tests were not needed.

Happy to do the other changes as soon as possible!

@DrTVockerodtMO DrTVockerodtMO added enhancement New feature or request optimisation This PR adds optimisations or edits OpenMP code labels Jul 29, 2026
@mike-hobson

Copy link
Copy Markdown
Contributor

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 adjoint_tests app present. We collectively agreed early on that this would be the way to test the adjoint and that unit tests were not needed.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request optimisation This PR adds optimisations or edits OpenMP code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert transport PSyAd kernels and algorithms into source code

6 participants