Radaer api la side - part one run time - #636
Conversation
iboutle
left a comment
There was a problem hiding this comment.
Hi Alan,
Thanks for showing clearly where the KGO changes come from - I'm now happy with this.
Just one minor comment on the code itself below.
Cheers
Ian
|
|
||
| ! pressure on theta levels | ||
| real(r_um),dimension( row_length, rows, nlayers ) :: p_theta_levels | ||
| real(r_um),dimension( row_length, rows, nlayers ) :: p_theta_levels_um |
There was a problem hiding this comment.
Rather than adding _um here, I would suggest removing it here and elsewhere from variable names - there is now really nothing to do with the UM in this code, so labelling fields with it makes no sense. I don't think they need any subscript after because they are all unique anwyay.
There was a problem hiding this comment.
this is probably a legacy thing. occasionally we had to distinguish variables that were both r_def and r_um. I will get rid of it. Maybe at some point we will rename r_um
Mohit Dalvi (mcdalvi)
left a comment
There was a problem hiding this comment.
Minor comments, but agree with Ian that the '_um' should be dropped. I expect this is for mapping to the names inside the core ''ukca_radaer_xx'' routines?
| @@ -389,6 +391,8 @@ subroutine radaer_code( nlayers, & | |||
|
|
|||
| use ukca_option_mod, only: do_not_prescribe | |||
|
|
|||
| use ukca_radaer_lfric_api_mod, only: ukca_radaer_lfric_interface | |||
There was a problem hiding this comment.
Subroutine needs to be accessed via ukca_api_mod as per the API design.
There was a problem hiding this comment.
We spoke offline and agreed that this is not required because this is separate from the UKCA API.
|
|
||
| if ( .not. associated( aod_ukca_ait_sol, empty_real_data ) ) then | ||
| do k = 1, npd_ukca_aod_wavel | ||
| do i = 1, row_length | ||
| aod_ukca_ait_sol( map_aod_wavel(i) + k - 1 ) = & | ||
| aod_ukca_this_mode_um(i,k) | ||
| aod_ukca_all_modes_um(i,k,mode_ait_sol-1) |
There was a problem hiding this comment.
Same comment as the UKCA-side change, could use of '-1' for the 3rd index be made more robust? Is there a difference in start index 0 vs 1 in the arrays?
There was a problem hiding this comment.
My understanding is this arises from the fact that radaer excludes the 1st UKCA mode (nucleation) and so operates on modes 2-7 in the UKCA indexing, but accommodates these in arrays of size 6, hence the -1. The only way this is likely to become an issue is if radaer wanted to include the nucleation mode ever - but I imagine lots of hard-wired things might break if we wanted to do that!
There was a problem hiding this comment.
Ian is correct. Nucleation has almost zero impact on radiation properties, so we will never use nucleation mode in RADAER.
There was a problem hiding this comment.
Thanks Alan (& Ian). It will help to add a comment for this, something like "to adjust indexing for soluble nucleation mode values that are at the first position in the array but not used here"
|
Thanks Alan. Sci-tech Review passed |
Mohit Dalvi (mcdalvi)
left a comment
There was a problem hiding this comment.
Sci-tech Review passed
PR Summary
Sci/Tech Reviewer: Mohit Dalvi (@mcdalvi)
Code Reviewer: Yaswant Pradhan (@yaswant)
I developed the original lfric to radaer functionality in 2020. Like a lot of developments back then, it was needed in a hurry and so I developed a fudged solution with technical debt.
We will need user flexibility soon to be able to run radaer with different user settings, hence this part one of two development.
In part one, I have introduced the user flexibility that is required for run time. Its already a very large change, and this is a natural break point for splitting a big PR.
Some hard-coded settings remain in the development (on the lfric_apps side the lists used by the case statement), which will be removed in the part two ticket to develop the initialisation stage functionality.
The KGO changes result from differences in 32bit r_def and 64bit r_um variables being added together in a slightly different way. Revision ab953f8 , which preserved the old adding method maintains KGO (see run1).
https://cylchub/services/cylc-review/taskjobs/alan.j.hewitt/?suite=radaer_api_la_side_3%2Frun1
Revision 4bf2d99, makes changes to the new case statement adding method which cause bit level differences.
4bf2d99
In hindsight, I should have wrapped all the elements of the old adding method with real( ... , r_um) as there would not have been a KGO change here.
linked Radaer api ukca side - part one run time ukca#41
blocks Radaer api la side - part two initialisation #683
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - radaer_api_la_side_3/run2
Suite Information
Task Information
✅ succeeded tasks - 1594
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