-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Harmonise variable names from UM Stash to LFRic variable and long_name #898
Harmonise variable names from UM Stash to LFRic variable and long_name #898
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and the test makes sense. It might be worth getting @jfrost-mo to have a quick look at it from a more detailed technical perspective. Also needs rebasing to be up-to-date with main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking like a good change. My main comment is to better handle the case where the STASH code isn't in the STASH_TO_LFRIC
dictionary.
Once you've addressed the comments I'll re-review.
Agreed Co-authored-by: James Frost <[email protected]>
Agreed Co-authored-by: James Frost <[email protected]>
Agree with James' further technical suggestions, and have committed those to branch. Have successfully rerun tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Just one minor comment, and there appears to be one test failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just needs one more test to make sure the exception is raised when no stash is present works then it will be good to go.
New test added, and checked working as intended. Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and makes sense to me. @jfrost-mo do you want one final check before approval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I've commented with a couple minor documentation tweak, but happy for you to merge when done.
Co-authored-by: James Frost <[email protected]>
Co-authored-by: James Frost <[email protected]>
Adding variable name callback to make use of LFRic variable standard names across CSET
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.