Add an io_value type for integers#326
Add an io_value type for integers#326Steve Mullerworth (stevemullerworth) wants to merge 12 commits into
Conversation
allynt
left a comment
There was a problem hiding this comment.
This PR correctly extends io_value_type to store integers as well as reals (r_def). It does this by creating a new type just for integers, integer_io_value_type, and then adding interfaces where distinguishing between reals and integers is required.
LGTM.
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
Thanks Steve Mullerworth (@stevemullerworth), this mostly looks ready to head into testing but I have one small suggestion regarding a code block which is repeated throughout. I'm hoping I've understood the technical aspect of this correctly, but let me know if I have missed context.
| array_dims = size(io_value%data) | ||
| if ( xios_is_valid_field(trim(value_id)) ) then | ||
| ! Integers must be converted to XIOS real kind | ||
| allocate(dp_equiv(array_dims)) | ||
| dp_equiv = real(io_value%data,dp_xios) | ||
| call xios_send_field( trim(value_id), & | ||
| reshape(dp_equiv, (/ 1, array_dims /)) ) | ||
| deallocate(dp_equiv) | ||
| else | ||
| call log_event( 'No XIOS field with id="'//trim(io_value%io_id)//'" is defined', & | ||
| LOG_LEVEL_ERROR ) | ||
| end if |
There was a problem hiding this comment.
Given that this is a repetition of the XIOS real conversion above, would it make more sense to move this into a small helper subroutine to improve readability and future maintenance ?
There was a problem hiding this comment.
The io_value object is an abstract type. The select type disambiguates between either io_value_type or integer_io_value_type so that within the first block io_value behaves as an io_value_type but within the second it behaves as an integer_io_value_type. So two helper subroutines would be needed to satisfy the two types!
There was a problem hiding this comment.
Thanks Steve Mullerworth (@stevemullerworth), this is super helpful, I may need to pick your brains some more to understand this further but for now I'm happy for this to head into testing and then onto main :) .
| array_dims = size(io_value%data) | ||
| if ( xios_is_valid_field(trim(checkpoint_id)) ) then | ||
| allocate(dp_equiv(array_dims)) | ||
| dp_equiv = real(io_value%data, dp_xios) | ||
| call xios_send_field( trim(checkpoint_id), & | ||
| reshape(dp_equiv, (/ 1, array_dims /)) ) | ||
| deallocate(dp_equiv) | ||
| else | ||
| call log_event( 'No XIOS field with id="'//trim(checkpoint_id)//'" is defined', & | ||
| LOG_LEVEL_ERROR ) | ||
| end if |
There was a problem hiding this comment.
Similarly to the last comment the moving of this block to a subroutine would benefit the code here.
There was a problem hiding this comment.
Same reply as above
| array_dims = size(io_value%data) | ||
| if ( xios_is_valid_field(trim(checkpoint_id)) ) then | ||
| allocate(dp_equiv(array_dims)) | ||
| dp_equiv = real(io_value%data, dp_xios) | ||
| call xios_send_field( trim(checkpoint_id), & | ||
| reshape(io_value%data, (/ 1, array_dims /)) ) | ||
| reshape(dp_equiv, (/ 1, array_dims /)) ) | ||
| deallocate(dp_equiv) | ||
| else | ||
| call log_event( 'No XIOS field with id="'//trim(checkpoint_id)//'" is defined', & | ||
| LOG_LEVEL_ERROR ) | ||
| end if |
There was a problem hiding this comment.
Replacement also needs to happen here.
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
New context about the repeated code blocks has now been clarified by the developer, and therefore I'm happy to put this into testing.
PR Summary
Sci/Tech Reviewer: allynt
Code Reviewer: Pierre Siddall (@Pierre-siddall)
Currently, support for storing real arrays in the checkpoint dump is provided by the
io_value_typeand associated support routines in the LFRic-XIOS component. This change adds support for storing and checkpointing integer arrays.As it changes the existing API, a linked MetOffice/lfric_apps#427 PR must be committed alongside this one.
As XIOS does not have an API for writing integer data, integer values are converted to
dp_xioswhich would normally be areal64value. This is accurate for allint32numbers, but would not be shoulddp_xiosever bereal32(precision is insufficient for integer values greater than or equal to 2 to the power 24). The unit tests were written to check for this possibility.Note on implementation 1: Currently,
io_value_typeis hardwired tor_defwhich can bereal64orreal32depending on the model configuration. Therefore,r_defis explicitly stated in the updated read/write API in the LFRic-XIOS component. Like with integers, there is a conversion todp_xioswhich would have no impact on values whetherr_defis 32-bit or 64-bit.Note on implementation 2: The writing/reading of fields of different types/kinds is done through a single generic routine, such as
write_field_generic, which includes aselect typeto differentiate the types/kinds. This setup is possible because thewrite_interfaceAPI in the commonfield_parent_typerelates to thefield_parent_proxyrather than thefield_parent_type. As the IO value types do not have an equivalent proxy, the analogous write interface must refer to the specific object itself. In other words, the integer andr_defread and write interfaces take the actualinteger_io_value_typeorio_value_typeas arguments, requiring the code to define specific subroutines for reading/writing.As the read and write APIs should have different intent (the write interface should have
intent(in)), the existing abstract interface that was shared has been split into two.As indicated above, the change also adds some unit tests for the LFRic-XIOS capabilities for both real and integer arrays as previously no io_value testing existed. During this work, it was noted that the test data was not being allocated correctly, so this was tidied.
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_core - core_chkpt_seed/run2
Suite Information
Task Information
✅ succeeded tasks - 382
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