-
Notifications
You must be signed in to change notification settings - Fork 49
#81: Fix unallocated arrays in ukca_volcanic_so2
#133
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
base: main
Are you sure you want to change the base?
Conversation
b40195f to
043b2b2
Compare
|
Hi Mohit. Just assigning a code reviewer for you and noticed that this pull request is asking to merge changes into "Stable". You'll need to change that to "Main". This is done right a the top, just under the title of the PR. Thanks |
|
Testing passed - ready for Sci-tech Review @alanjhewitt |
alanjhewitt
left a comment
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.
Hi Mohit,
Thanks for making this change.
I think the clean error message could be clearer why its not working, because the next person to see this message will likely be a scientist rather than a software engineer, and we want to make it easy for them to figure out what is wrong.
Keep it brief but say that it needs to be implemented before the code will work.
Alan
| ! Functionality is not currently implemented in LFRic. | ||
| errcode = 1 | ||
| call ereport(RoutineName, errcode, & | ||
| 'Functionality not available -'//RoutineName//' should not be called') |
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.
Could you change the error message, so that it says the functionality is not yet developed/implemented for lfric.
|
Error message updated for clarity - 49a8a2d |
alanjhewitt
left a comment
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.
Thanks Mohit, looks good. Approved
PR Summary
Sci/Tech Reviewer: @alanjhewitt
Code Reviewer: @oakleybrunt
Arrays
true_latitude,true_longitudeandweight_volc_vertdistare being used without being allocated in LFRic copy of ukca_volcanic_so2 routine. This might have been a fallout of migrating the UM physics code.This is causing CCE builds on ARCHER2 to fail, and warnings during the GNU compilation.
Note: the Volcanic_SO2 emission functionality is not yet fully linked up in LFRic. Hence this PR will also add a check to ensure that the routine is not called.
Issue: #81
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - fix_i81_ukca_volc/run2
Suite Information
Task Information
✅ succeeded tasks - 1106
Testing information from ARCHER2 is noted in the issue:81 Summary
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