Reworked Configuration Namelist Access API #175
Conversation
…mentation refers to.
mo-rickywong
requested review from
MatthewHambley
and removed request for
a team
github-actions
bot
added
cla-signed
There was a problem hiding this comment.
I notice that across the board you assign namelist values to variables before using them. Can you confirm this is a choice you've made and that, as functions, they can be used in-line. e.g. call something(value, modeldb%config%my_namelist%other_value().
Yes this is a choice, the values can be accessed in-line. I have extracted them as the variables existed and shortens the actual code so it appears cleaner.
| ----- | ||
| The Configurator provides several python scripts found in | ||
| ``infrastructure/build/tools``. Each of these scripts generate | ||
| Fortran source code that is specific to an applications metadata. |
There was a problem hiding this comment.
| Fortran source code that is specific to an applications metadata. | |
| Fortran source code that is specific to an application's metadata. |
There was a problem hiding this comment.
This conversation can be closed, the suggestion was applied to branch
There was a problem hiding this comment.
I presume there is some deprecation around things appearing in this document. A note of that would be helpful. What is the preferred way, what is deprecated?
The documentation has a section on deprecated process. I'll review this file for deprecated bits
| In general, applications will only require one ``config_type`` object. | ||
| Should an application require to read more than one configuration file | ||
| in a model run, another ``config_type`` will be required, | ||
| `i.e.` 1 ``config_type`` object/configuration file. |
There was a problem hiding this comment.
What is the last line trying to say? Do you mean one config_type per configuration file?
yes, was trying to explain without pointing the finger at specific groups.
There was a problem hiding this comment.
This was removed/relocated so that the multiple configuration issue is referred to under loading the configuration
| source_partitioner) | ||
| call destination_partitioning_nml%get_value('partitioner', & | ||
| destination_partitioner) | ||
| call iter%initialise(config%partitioning) |
There was a problem hiding this comment.
Is there an issue for providing a convenience function to directly recover a namelist by key? Are there problems with providing such a convenience function?
There was a problem hiding this comment.
No problem I don't think, it was rejected at the team meeting when discussed, though lfricinputs are aparently trying to do this.
| namelists may have multiple instances or when an application wishes | ||
| to load multiple configuration files. It is recommended that access | ||
| to configuration namelists be limited to usage of the ``modeldb%config`` | ||
| item described above. |
There was a problem hiding this comment.
This may be the place to put an actual date on when the old implementation will be removed.
There was a problem hiding this comment.
I'd rather not put a date on here yet, until we have a fair idea that the rest of the piecemeal changes are just cranking the handle. Attempts to update the LFRic_apps repo present some issues that I'd like to iron out before putting a date in that might change.
There was a problem hiding this comment.
Actually, changed my mind on that, put a date of April 2026, so we can pull the the plug on it any time after.
| !> @param [in] namelist The namelist that is to be added | ||
| !> into the collection. |
There was a problem hiding this comment.
Is there something more useful to be said about the namelist, rather than repeating the brief description? Are there any restrictions or requirements on it?
There was a problem hiding this comment.
Updated, the namelist type needs to be one that is defined in the application metadata otherwise it won't be installed in the config object.
| character(str_def) :: full_name | ||
|
|
||
| ! Check namelist name is valid, if not then exit with error | ||
| full_name = namelist_obj%get_full_name() |
There was a problem hiding this comment.
Can strings be handled as character(:), allocatable thereby holding only the payload? Remember allocatable character arrays are magic and work with the = operator much better than any other type of allocatable.
| {% endif %} | ||
| {%- endfor %} | ||
|
|
||
| {%- for i in range(namelists|length) %} | ||
| {%- if duplicates[i] %} |
There was a problem hiding this comment.
Can this be done as one loop? Loop over namelists, if duplicates is true, do one thing, else (it's false) do the other.
There was a problem hiding this comment.
It could, though it was done this way so that all the namelists that allow multiple instances are grouped together in the resulting config_type definition.
|
|
||
| end function {{parameter.name}} | ||
|
|
||
| {%- else %} |
There was a problem hiding this comment.
It can be good to add a comment to else clauses which gives the logic which triggers it, in this case name not in arrays. If Jinja allows for comments within curly brackets.
There was a problem hiding this comment.
Comments added
| $(Q)mkdir -p $(dir $@) | ||
| $(Q)$(LFRIC_BUILD)/tools/GenerateLoader $(VERBOSE_ARG) $@ $(shell cat $(CONFIG_DIR)/config_namelists.txt) | ||
|
|
||
| $(Q)touch $(WORKING_DIR)/duplicate_namelists.txt |
There was a problem hiding this comment.
You might move this down to where it is used, unless it is secretly used by GenerateConfigLoader and GenerateExtendedNamelistType.
There was a problem hiding this comment.
Moved it earlier on, to after GenerateNamelistLoader. this script will produce duplicate_namelists.txt. The touch is to ensure that is a file to find for GenerateConfigType
Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>
Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>
…f90.jinja Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>
…f90.jinja Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>
Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>
mo-rickywong
requested review from
EdHone,
andrewcoughtrie and
mike-hobson
as code owners
MatthewHambley
left a comment
MatthewHambley
left a comment
There was a problem hiding this comment.
Not too many changes, some of which may be postponed to later sets.
| $(Q)$(LFRIC_BUILD)/tools/GenerateNamelistLoader \ | ||
| $(VERBOSE_ARG) \ | ||
| $(CONFIG_DIR)/rose-meta.json \ |
There was a problem hiding this comment.
If you're reformatting this command please pick a continuation marker style and stick with it. Either marker goes immediately after the end of the line (with a space) or they are lined up. This seems to be neither.
|
|
||
| def __init__(self, intrinsic_type: str, kind: str, write_format: str): | ||
| """ | ||
| :param intrinsic_type: One of "integer", "real", etc. |
There was a problem hiding this comment.
If this is a set list, why not use an Enum?
There was a problem hiding this comment.
Not sure I follow fully, this file is a duplicate of namelistdescription.py with some changes for the extended namelist. I don't fully understand the query, though I assume it would also apply to the original namelistdescription.py file. So I guess the author of that file would know better than I.
|
|
||
| _singletonMap: Dict[str, Dict[str, Dict[str, "FortranType"]]] = {} | ||
|
|
||
| def __init__(self, intrinsic_type: str, kind: str, write_format: str): |
There was a problem hiding this comment.
"Kind" probably needs to be optional as we do not require one with integers.
| """ | ||
| Gets the type designator used by declarations in source files. | ||
| """ | ||
| return f"{self.intrinsic_type}({self.kind})" |
There was a problem hiding this comment.
This will need to take into account optional kinds.
| """ | ||
| Gets a label for this type. | ||
| """ | ||
| return f"{self.intrinsic_type}_{self.kind}" |
There was a problem hiding this comment.
Kind may be optional.
| select type(namelist_obj) | ||
| class default | ||
| write(log_scratch_space, '(A)') & | ||
| ' Undefined namelist type(' // trim(name) // & | ||
| '), for this configuration.' | ||
| call log_event(log_scratch_space, log_level_error) | ||
|
|
||
| end select |
There was a problem hiding this comment.
Don't you want to make this check before trying to dereference the object to get the various name strings?
| !> @brief Queries config_type for the total number of namelists stored. | ||
| !> @return answer The number of namelists stored | ||
| !===================================================================== | ||
| function n_namelists(self) result(answer) |
There was a problem hiding this comment.
While words for symbol names.
| implicit none | ||
|
|
||
| class(config_type), intent(in) :: self | ||
| character(str_def) :: answer |
There was a problem hiding this comment.
| character(str_def) :: answer | |
| character(:), allocatable :: answer |
There is magic around assigning strings which means this should work.
|
|
||
| character(*), optional, intent(in) :: listname | ||
|
|
||
| character(str_def), allocatable :: namelist_names(:) |
There was a problem hiding this comment.
I think you are correct that you cannot have allocatable arrays of allocatable values. It's all covered in the documentation you are down to review at MetOffice/Technical-Knowledgebase#5.
| if (allocated(self%nml_fullnames)) deallocate(self%nml_fullnames) | ||
|
|
||
| self%config_name = cmdi | ||
| self%isinitialised = .false. |
There was a problem hiding this comment.
Normally "clear" semantics would not de-initialise the object. That would be something the destructor would do. Is there a reason for it here?
2 participants
PR Summary
Sci/Tech Reviewer: @allynt
Code Reviewer: @MatthewHambley
This PR is to allow users more direct access to the namelist configuration values from
Fortran object (rather than global module scope) while maintaining it's read-only nature.
e.g.
modeldb%config%<MyNamelist>%MyNamelistMember()This PR is linked to LFRic-core trac ticket #4702, which provides more details on the change itself
Code Quality Checklist
(Some checks are automatically carried out via the CI pipeline)
style guidelines
readability of the code
Testing
using this branch
acceptable (eg. kgo changes)
tests, unit tests, etc.)
and have been allocated to an appropriate testing group (i.e. the
developer tests are for jobs which use a small amount of compute resource
and complete in a matter of minutes)
trac.log
Security Considerations
Performance Impact
performance measurements have been conducted
AI Assistance and Attribution
of Generative AI tool name (e.g., Met Office Github Copilot Enterprise,
Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the
Simulation Systems AI policy
(including attribution labels)
Documentation
confirmed that it builds correctly
PSyclone Approval
interface, optimisation scripts, LFRic data structure code) then please
contact the
tooscollabdevteam@metoffice.gov.uk
Sci/Tech Review
Please alert the code reviewer via a tag when you have approved the SR
Code Review