Separate model checksum and finalise#441
Separate model checksum and finalise#441Steven Sandbach (ss421) wants to merge 4 commits intoMetOffice:mainfrom
Conversation
Updated via: 3aa8f09 |
| prognostic_fields => modeldb%fields%get_field_collection( & | ||
| "prognostic_fields") | ||
| diagnostic_fields => modeldb%fields%get_field_collection( & | ||
| "diagnostic_fields") |
There was a problem hiding this comment.
Are diagnostic fields used?
There was a problem hiding this comment.
They do not appear to be used. Removed via: 20c12fc
| moisture_fields => modeldb%fields%get_field_collection("moisture_fields") | ||
| call moisture_fields%get_field("mr", mr_array) | ||
| mr => mr_array%bundle | ||
| fd_fields => modeldb%fields%get_field_collection("fd_fields") |
There was a problem hiding this comment.
They do not appear to be used. Removed via: 20c12fc
| call prognostic_fields%get_field('theta', theta) | ||
| call prognostic_fields%get_field('u', u) | ||
| call prognostic_fields%get_field('rho', rho) | ||
| call prognostic_fields%get_field('exner', exner) |
There was a problem hiding this comment.
Its curious that exner is listed here, but not actually used in the checksum. Ideally it should be added. But that would mean ALL the kgos would change. Its probably not necessary, given that we've used this code for a long time. Perhaps a comment can be added.
There was a problem hiding this comment.
Added a comment in: 20c12fc. I could open an issue and reference it via the comment? I dont mind making that change but equally could assign the issue to someone - can you advise?
There was a problem hiding this comment.
I discussed with others in dynamics research and we agree that given that we've managed so far without exner in the checksum, its not a priority to add it in. And therefore best to just leave as is. But thanks for adding the comment to highlight that its not actually used in the checksum.
cjohnson-pi
left a comment
There was a problem hiding this comment.
As Sci/Tech reviewer, I've had a more thorough look and have added a few comments.
Thanks for the review - apologies for adding you as Sci-Tech review before you agreed. I did that because you had reviewed the code and previously discussed the change I made the assumption... I've updated as suggested and am rerunning the test suite via: |
All test are now running. I did need to make an extra commit: d986f6a because I left whitespace in the previous commit. Also note a number of tasks failed but on re-trigger they were successful. Let me know if there is anything else you would like me to do. |
PR Summary
Sci/Tech Reviewer: cjohnson-pi
Code Reviewer:
This is a small followup refactor that I suggested in: https://code.metoffice.gov.uk/trac/lfric/ticket/4351 (and copied to: #439). It separates the finalization and checksum of the model. The motivation here is for slightly cleaner interface as the checksum is only needed with running the full model and is not needed in the integration tests and JEDI.
The current solution is the inclusion of an optional that does the checksum if it is present. Here the checksum is explicitly called.
Code Quality Checklist
Testing
Tested via:
cylc vip -z group=developer -S USE_MIRRORS=true -n gungho_finalise_model ./rose-stemand the trac.log is copied below.trac.log
Test Suite Results - lfric_apps - gungho_finalise_model/run1
Suite Information
Task Information
✅ succeeded tasks - 1164
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