Replace 2-elements sequences by Pair utility for double buffering
#590
Conversation
egparedes
force-pushed
the
use_double_buffer_class
branch
from
9290dfa
to
ec37607
Compare
|
cscs-ci run default |
| theta_v_now=prognostic_state_swp.current.theta_v, | ||
| exner_new=prognostic_state_swp.other.exner, |
There was a problem hiding this comment.
Since Swapping is part of the application code, we could use current and next or even now and next instead of current and other.
other is perfect for a generic Swapping but less readable in the application code where this is used for timestepping.
There was a problem hiding this comment.
Refactored Swapping to generic Pair which can be easily subclassed to set accessors with custom names.
egparedes
changed the title
Swapping utility class for double buffersPair utility subclasses for double buffers
Co-authored-by: Magdalena <luzm@ethz.ch>
|
cscs-ci run default |
egparedes
changed the title
Pair utility subclasses for double buffersPair utility for double buffering
|
cscs-ci run default |
There was a problem hiding this comment.
LGTM, some very minor things. I still haven`t fully understood the swapping for the diagnostic tendencies. But I think we should try to fully understand that with @OngChia s help and fix or remove swapping/Pair where not needed. That should become an extra PR.
model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/solve_nonhydro.py
Outdated
Show resolved
Hide resolved
|
The CI failure is due to some corrupt data unrelated to your PR. |
|
cscs-ci run default |
…n the diagnostic state
|
cscs-ci run default |
1 similar comment
|
cscs-ci run default |
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
In case your change might affect downstream icon-exclaim, please consider running
For more detailed information please look at CI in the EXCLAIM universe. |
|
cscs-ci run default |
| """ | ||
| Set time levels of ddt_adv fields for call to velocity_tendencies. | ||
|
|
||
| When using `TimeSteppingScheme.MOST_EFFICIENT` (itime_scheme=4 in ICON Fortran), |
There was a problem hiding this comment.
Sorry. I was wrong in my previous comment. I overlooked the existence of l_recompute. This option is only True at first substep. Hence, run_predictor_step of velocity_advection is not run from the second substep onwards.
You can add ddt_v_apc.current is computed at every first substep as follows.
| When using `TimeSteppingScheme.MOST_EFFICIENT` (itime_scheme=4 in ICON Fortran), | |
| When using `TimeSteppingScheme.MOST_EFFICIENT` (itime_scheme=4 in ICON Fortran), | |
| `ddt_w_adv_pc.current` is not computed in the predictor step of each substep. | |
| Instead, the value computed in the corrector step during the previous substep is reused | |
| for efficiency (except, of course, in the very first substep of the | |
| initial time step). | |
| `ddt_v_apc.current` is only computed in the predictor step of the first substep and the value | |
| in the corrector step during the previous substep is reused for `ddt_v_apc.current` from the second substep onwards. |
| the predictor and .next for the corrector) and interpoolated at the end | ||
| of the corrector step to get the final output. | ||
|
|
||
| If a different time stepping scheme is used, the first element of the pair |
There was a problem hiding this comment.
I do not get the meaning of this comment.
For the most inefficient itime_scheme=6, there is no need to swap because everything is recomputed at every substep. But we still need two buffers for ddt_vn_apc_pc and ddt_w_adv_pc and interpolate them at the corrector step to update (the interpolation coefficient is determined by veladv_offctr which is set in the NonHydrostaticConfig). Unless we use itime_scheme<4, then we only need to have one buffer for ddt_vn_apc_pc and ddt_w_adv_pc. I am not sure how many itime_scheme icon4py supports.
For itime_scheme=5, one still has to swap these variables.
Currently, no other time stepping scheme is supported.. Why do we need this statement? I would suggest
| If a different time stepping scheme is used, the first element of the pair | |
| No other time stepping schemes are currently supported. |
| else: | ||
| self.ntl1 = 0 | ||
| self.ntl2 = 0 | ||
| # Use only the first item of the pairs for both predictor and corrector outputs |
There was a problem hiding this comment.
I saw that currently we only support TimeSteppingScheme.MOST_EFFICIENT. And Anurag and I think that the next one should be itime_scheme=6 because it is the real predictor-corrector scheme without all this assumptions to improve efficiency. "Use only the first item of the pairs" is only valid if we also support itime_scheme<4. And there is no test for it...
I just feel that the comment here is a bit misleading here.
Simply throw an error here if self._config.itime_scheme is not TimeSteppingScheme.MOST_EFFICIENT
| self.ntl2 = nnew | ||
| if not at_first_substep: | ||
| diagnostic_state_nh.ddt_w_adv_pc.swap() | ||
| diagnostic_state_nh.ddt_vn_apc_pc.swap() |
There was a problem hiding this comment.
I think this is wrong. ddt_w_adv_pc needs to be swapped at every substep except the first substep of first time step (I guess all CI datatest will still pass because no datatests run two consecutive time steps even if it is wrong here...).
ddt_vn_apc_pc is correct. It does not need to be swapped at every first substep.
| diagnostic_state_nh.ddt_vn_apc_pc.swap() | |
| if self._config.itime_scheme == TimeSteppingScheme.MOST_EFFICIENT: | |
| if not l_init: | |
| diagnostic_state_nh.ddt_w_adv_pc.swap() | |
| if not at_first_substep: | |
| diagnostic_state_nh.ddt_vn_apc_pc.swap() |
Update
icon4py_driverto use thePairutility classes and add a pair ofNamedTuples to document the multiple values returned by theinitializefunction.