Remove post processing core#372
Conversation
…ing. functional switching; only post process on ugrid planar projected. XIOS3 compatibility.
mo-marqh
requested review from
Mike Hobson (mike-hobson)
and removed request for
a team
|
there are 4 failing This is the expected change from this change Set. It is not clear whether a linked PR is needed here, as the code and tests would not change to let these pass, instead the static files that support these this code is ready for Sci/Tech review, but this lfric_apps testing coordination needs addressing before it passes cove review. |
|
Just to comment that from a user perspective we're happy with the approach here. The xios post-processing to add the forecast_reference_time & forecast_period was only applied to the specific, hard-wired file "lfric_diag.nc". Most users have moved away from using this file due to problems with how it is constrained and issues caused by the post processing, favouring instead files which are defined directly to xios in the xml files. As these files never had the post processing applied anyway, they will see no change from this PR, therefore it's expected to have no impact on downstream users. |
|
updated with linked PR MetOffice/lfric_apps#510 |
| file_convention_ugrid, & | ||
| file_convention_cf | ||
| use lfric_mpi_mod, only: global_mpi | ||
| use lfric_ncdf_dims_mod, only: lfric_ncdf_dims_type |
There was a problem hiding this comment.
Unused dependency, can be removed now
| @@ -21,24 +26,21 @@ | |||
| use lfric_ncdf_field_group_mod, only: lfric_ncdf_field_group_type | |||
There was a problem hiding this comment.
Unused dependency, can be removed now
| FILE_OP_OPEN | ||
| use io_config_mod, only: file_convention, & | ||
| file_convention_ugrid, & | ||
| file_convention_cf |
There was a problem hiding this comment.
file_convention_cf is no longer used, can be removed
| !> | ||
| module lfric_xios_process_output_mod | ||
|
|
||
| use constants_mod, only: i_def, r_def, str_def |
There was a problem hiding this comment.
r_def and str_def can be removed
| if (global_mpi%get_comm_rank() /= 0) return | ||
|
|
||
| call log_event("Processing output file: "//trim(file_path), log_level_trace) | ||
| call log_event("Processing output file: "//trim(file_path), log_level_info) |
There was a problem hiding this comment.
We should consider here whether info is the correct level of logging. I think it's okay as it's close to the end of the run and not being called many times but it's always good to consider the options, potentially debug would also be fine.
There is also the issue of there being a code path here (if the file convention is not UGRID) where the log message will be called but the information it gives will be incorrect, as the file wont be processed. Potentially the message could be moved?
| logical :: xios_context_initialised = .false. | ||
| !> Flag denoting if this file is a UGRID Planar mesh file with | ||
| !> projected coordinates that have been scaled | ||
| logical :: ugrid_scaled_projected_coordinates = .false. |
There was a problem hiding this comment.
Should just be the one space here I think!
| file_convention == file_convention_ugrid ) then | ||
| this%ugrid_scaled_projected_coordinates = .true. | ||
| end if | ||
| if (this%filelist%get_length() > 0) call setup_xios_files(this%filelist, & |
There was a problem hiding this comment.
The logic here is a little strange, with the attachment of the logical to the various lfric_xios classes in order to pass the information through. As it's a (hopefully) temporary solution I think it's fine for now. A fix for this issue within XIOS is probably something we should highlight as a future development in XIOS.
| end do | ||
| ! Only take action if this is a regional model with UGRID Projected | ||
| ! coordinates, as these are awaiting XIOS feature development | ||
| if ( this%ugrid_scaled_projected_coordinates ) then |
There was a problem hiding this comment.
Are we testing for the ugrid_scaled_project_coordinates logic twice? If so, it is probably unecessary and just testing at the context level is probably sufficient
| logical :: diag_always_on_sampling = .true. | ||
| !> Flag denoting if this file is a UGRID Planar mesh file with | ||
| !> projected coordinates that have been scaled | ||
| logical :: ugrid_scaled_projected_coordinates = .false. |
There was a problem hiding this comment.
See above - possibly we don't also need to test this logic at the individual file level
| call log_event( "Waiting for XIOS to close file ["//trim(self%path)//".nc]", & | ||
| log_level_debug ) | ||
| call init_wait() | ||
| call log_event( "post processing file ["//trim(self%path)//".nc]", & |
There was a problem hiding this comment.
In the past we were reluctant to use the term post-processing as this because confused with the later stages of data post processing of down stream data products, so we just used processing. Also, please could you capitalise the first letter of the log message
4 participants
PR Summary
Sci/Tech Reviewer: Ed Hone (@EdHone)
Code Reviewer: Steve Mullerworth (@stevemullerworth)
This change removes the post-processing logic for the majority of cases from
coreinit_waitcalls.lfric_appsmetadata definitions have meant that the use of the code is sporadic and not well understood with regard to triggers withinlfric_appsIn detail:
lfric_appsCode Quality Checklist
Testing
trac.log
Test Suite Results - lfric_core - remove-post-processing-core/run2
Suite Information
Task Information
✅ succeeded tasks - 402
also tested with
lfric_appsmainto confirm compatibilityTest Suite Results - lfric_apps - test_apps_remove-post-processing-core/run1
Suite Information
Task Information
❌ failed tasks - 4
⌛ waiting tasks - 2
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