-
Notifications
You must be signed in to change notification settings - Fork 58
Update linear integration tests #170
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
|
test |
tommbendall
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 PR does quite a lot of things, and I have to admit to finding it difficult to navigate!
One of the larger changes seems to be to include moisture in several of the test algorithms, such as tl_test_rhs_alg_mod.x90 and tl_test_semi_imp_alg_mod.x90. There are a couple of changes that I don't understand, so have left comments on them.
It's an welcome change to use the appropriate driver level code in the control routines for the integration tests.
I appreciate the level of instructions in plot_convergence.sh. I checked out a copy of this branch and ran the script. It seems to take a very long time and hasn't finished yet -- I will report back it this fails.
| @@ -123,10 +123,13 @@ module gungho_step_mod | |||
| call log_event( log_scratch_space, LOG_LEVEL_INFO ) | |||
|
|
|||
| temp_corr_io_value => get_io_value( modeldb%values, 'temperature_correction_io_value') | |||
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 seems sensible to wrap the lines below in an if statement. Would it also make sense to wrap this line in an if statement about the energy correction?
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.
Now included
| @@ -0,0 +1,112 @@ | |||
| ############################################################################## | |||
| # (c) Crown copyright 2022 Met Office. All rights reserved. | |||
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.
Should the year be 2026?
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.
I've just moved this script from another directory, so the origin date is correct.
| #!/usr/bin/env python | ||
| # -*- coding: utf-8 -*- | ||
| ############################################################################## | ||
| # (c) Crown copyright 2021 Met Office. All rights reserved. |
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.
Should the year be 2026?
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.
as above
| echo "Do not need to build the executable as $exe exists" | ||
| else | ||
| echo "$exe does not exist, so now building the executable" | ||
| cd ../../../build |
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.
Does this mean we could only run this script from the plot_convergence directory? Could this be an absolutely path instead of a relative one?
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.
Absolute paths created
| plt.ylabel('Relative error') | ||
| plt.title('Validity of the tangent linear model') | ||
|
|
||
| #plt.show() |
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.
I don't know if you want to remove this comment? Or leave it in with an instruction to uncomment to show the plots?
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.
An extra comment added
| !> @param[in] norm_diff_prev Previous norm | ||
| !> @param[inout] pass_str Pass string (either PASS or FAIL) | ||
| !> @param[in] tol Test Tolerance | ||
| subroutine convergence_pass_string( norm_diff, norm_diff_prev, pass_str, tol ) |
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 this convergence_pass_string routine be removed, and could this file import the public routine from tl_test_convergence_rate_check.f90 instead? And if not, could there be an explanation here for the extra implementation?
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.
A brief explanation has been added to the description. What has happened is the original code applied the square roots to the norms before checking in the convergence test. But I improved this so that when there are multiple variables, the convergence rate is checked for each variable and for the sum (but this needs the square root to be done after). But I kept this original code to allow the plot convergence graphs to be created.
| ! Clean up solvers and timestep method left over from running | ||
| ! the model to set up model state and linear state | ||
| call final_si_operators() | ||
| call semi_implicit_solver_alg_final |
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.
| call semi_implicit_solver_alg_final | |
| call semi_implicit_solver_alg_final() |
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
| setval_X( exner, n1_exner ) ) | ||
| do i = 1, nummr | ||
| call invoke( setval_X( mr(i), n1_mr(i) ) ) | ||
| enddo |
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.
| enddo | |
| end do |
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
| setval_X(ls_state(igh_d), ls_rho ), & | ||
| setval_X(ls_state(igh_p), ls_exner) ) | ||
|
|
||
| call invoke( assign_field_random_kernel_type( random(igh_u), 1.0_r_def ) , & |
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 these removed because the incoming u, exner, etc fields are already initialised to random values? I'm struggling to find where that happened
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.
yes the perturbations are now initialised from real data. This is better as we used to have set with random data and gamma values - and choosing the gamma values was tricky. The real data seems more robust.
| setval_X(r_theta, theta), & | ||
| setval_X(r_rho, rho ) ) | ||
|
|
||
| ! Overwrite the moisture fields with potential temperature |
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.
Why?
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.
explanation added
tommbendall
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 for the explanation. I'm happy with the changes and that my suggestions have been addressed.
I have two final suggestions:
- Can you delete the
applications/linear_model/plot_convergencefolder? - It would be nice to get the
plot_convergence.shscript working if executed from a different directory. I've put a suggestion on how to do this
| config_list=(nwp_gal9 semi_implicit runge_kutta) | ||
|
|
||
| # Define directories using the current working directory | ||
| export Linear_dir="$(dirname $PWD)" |
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.
So I think this command still prevents us from running the script from folders that aren't science/linear/plot_convergence. I realise this is not a big deal, but I think this suggestion will fix it:
| export Linear_dir="$(dirname $PWD)" | |
| SCRIPT_DIR="$(dirname "$(realpath "$BASH_SOURCE")")" | |
| export Linear_dir="$(dirname $SCRIPT_DIR)" |
I've checked that it at least starts running correctly with this change
|
Thanks for the helpful suggestions. Both addressed. |
tommbendall
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 for the responses, this passes science review. @mo-marqh this is ready for code review
ss421
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 has no impact on JEDI.
PR Summary
Sci/Tech Reviewer: @tommbendall
Code Reviewer: @mo-marqh
#115
Code Quality Checklist
Testing
Plot convergence results
#115 (comment)
trac.log
Test Suite Results - lfric_apps - update_integration_test/run1
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