Purge Intermediate Api access to configuration object from in favor of new config_type object#262
Purge Intermediate Api access to configuration object from in favor of new config_type object#262mo-rickywong wants to merge 42 commits intoMetOffice:mainfrom
Conversation
…mentation refers to.
Co-authored-by: Matthew Hambley <[email protected]>
Co-authored-by: Matthew Hambley <[email protected]>
…f90.jinja Co-authored-by: Matthew Hambley <[email protected]>
…f90.jinja Co-authored-by: Matthew Hambley <[email protected]>
Co-authored-by: Matthew Hambley <[email protected]>
There was a problem hiding this comment.
Presumed code owner's review, although I'm not sure which code.
Everything here looks fine. I like the fact that on average more lines are being removed than added which suggests the original goal of simplifying configuration is being achieved.
I've left a comment for the eventual code reviewer to consider and have one question here:
Why are we choosing to forgo whole English words by replacing configuration with config for arguments? We can change the type of an argument without changing its name.
Response by @mo-rickywong config significantly shortens references to the module/declarations and code to extract variables from the namelist without (I feel) making it vague. The other reason is that during the transition period from the old api to the new api both namelist_collection_type and config_type configuration objects will exist, which could confuse things in the transition if both declared types where called configuration
| ! if ( geometry == geometry_spherical .and. & | ||
| ! topology == topology_fully_periodic) then | ||
| ! stretch_factor = get_stretch_factor() | ||
| ! else | ||
| ! stretch_factor = 1.0_r_def | ||
| ! end if | ||
|
|
||
| ! inverse_rot_matrix = get_inverse_mesh_rotation_matrix() | ||
| ! to_rotate = get_to_rotate() |
There was a problem hiding this comment.
Adding commented out code?
There was a problem hiding this comment.
Oversite, left in developer/debugging code, now removed
| 'Arguments "names" and "success_mask" to function' //& | ||
| '"ensure_configuration" are different shapes.' | ||
| flush(6) | ||
| stop |
There was a problem hiding this comment.
(I just noticed this after apps #208 was brought to my attention, so please consider this just a comment/question): why is the standard log_event (which would do a 'more graceful' shutdown in case of an error like here) replaced with a write(6....; flush; stop?
There was a problem hiding this comment.
I understand the reason for this - The log level is set through configuration, so the logger has to be initialised after configuration is read in. Calling the logger before it is initialised would be undefined behaviour. However, where this happens elsewhere in the code, we don't just write to unit 6, as this is a little uncontrolled, too. What we do elsewhere is:
use, intrinsic :: iso_fortran_env, only : error_unit
write(error_unit, '(A)')'Arguments...
There was a problem hiding this comment.
Branch has been updated to use error_unit and output_unit
There was a problem hiding this comment.
Currently, when uninitialised, the logger will write to the error unit if called with log_level_error before the logger is initialised (along with a message saying the logger is uninitialised). This is mentioned in the documentation. So it is OK to use the logger for this error message, I think, rather than to assume that the logger initialisation is dependent on the configuration.
Note, I've created a PR to ensure the uninitialised-logger warning message and normal non-error log messages also go to the error unit.
|
Branch brought up to head, hardcoded units switched to error_unit and output_unit |
|
Due to the old commits, this branch will need an override in order to commit |
PR Summary
Sci/Tech Reviewer: @allynt
Code Reviewer: @oakleybrunt
This ticket purges lfric_core of instance of the intermediate api to configuration namelists. Previously this was via at namelist_collection_type generally initialised and located as a member of the modeldb_type from driver_modeldb_mod. Following the commit of PR #175, templating and infrastructure for config_type was put in place on lfric_core, but only so that PR #175 would remain a non-linked PR change.
This PR removes the remaining initialisation and usage of the namelist_collection_type (configuration) from lfric_core. This however will require linked changes to lfric_apps. Changes to lfric_apps have been limited to only allow it to work with this lfric_core purge.
Linked PR
Code Quality Checklist
Testing
Security Considerations
Performance Impact
AI Assistance and Attribution
No AI was used on this change
Documentation
N/A
PSyclone Approval
N/A
Sci/Tech Review
N/A
This is a technical change only and doesn't need a SciTech, however code owners may wish to comment if there are issues.
Code Review