-
Notifications
You must be signed in to change notification settings - Fork 49
Split mol with transport_efficiency #149
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
| if ( .not. transport_efficiency ) then | ||
|
|
||
| ! When we have multiple split steps, we need an intermediate field for the | ||
| ! field at the start of each substeps |
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.
"substep" instead of "substeps" in comment "field at the start of each substep"
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.
Corrected
| num_split_steps = get_num_split_steps(transport_metadata%get_splitting()) | ||
|
|
||
| ! When we have multiple split steps, we need an intermediate field for the | ||
| ! field at the start of each substeps |
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.
"substep" instead of "substeps" in comment "field at the start of each substep"
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.
Corrected
|
|
||
| call sum_flux%initialise( vector_space = vector_space_w2_ptr ) | ||
| call flux_last_step%initialise( vector_space = vector_space_w2_ptr ) | ||
| call flux_last_step%initialise( vector_space = vector_space_w2v_ptr ) |
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.
Are you hardwiring the flux_last_step to always be W2V? Is it possible to put in a check based on the splitting used? If not is it worth commenting that this is only suitable for testing VHV splitting?
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.
Comment added
jameskent-metoffice
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.
This work sets up the split transport option for the linear model. This is vital as it will speed up the MoL linear transport, but is also necessary for FFSL linear transport (when we come to implement it).
I "pre-reviewed" this work when we were on trac, and I can see that the developer has addressed any issues I had. I have left 3 small comments in the code and I will leave it up to the developer to decide whether they warrant any changes.
It is good that the split MoL transport is tested in the linear suite, and it is the default config. It is also good that the old 3D MoL is still tested. I have looked at the plots for the kgo change tests and everything is looking good.
I am happy to approve.
PR Summary
Sci/Tech Reviewer: James Kent @jameskent-metoffice
Code Reviewer: @mo-marqh
Associated with #108 and #113
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - test_split_mol/run15
Suite Information
Task Information
✅ succeeded tasks - 1106
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