Timeloop #308
Timeloop #308
Conversation
…4, 18, and 20, and utils
…d time step test for timeloop and non hydro multistep
|
cscs-ci run |
|
launch jenkins spack |
|
cscs-ci run |
|
launch jenkins spack |
There was a problem hiding this comment.
Overall looks good. Some small changes, and a lot of stuff, that we hope to fix with the cleanup project in this cycle (like SolveNonHyrdo interface...)
we should also have a discussion on how to really setup the time loop to make it run as linear as possible, but we can refactor it to that in a second round.
| ndyn_substeps: int = ( | ||
| 5 # ndyn_substeps is not a constant in ICON, it may be modified in restart runs | ||
| ) | ||
| apply_horizontal_diff_at_large_dt: bool = True # lhdiff_rcf |
There was a problem hiding this comment.
I like that you get rid of these ununderstandable abreviations, the diff stands for diffusion, so why not also write this in full? You could also add a docstring or comment docstring for the old icon name:
| apply_horizontal_diff_at_large_dt: bool = True # lhdiff_rcf | |
| apply_horizontal_diffusion_at_large_dt: bool = True #: lhdiff_rcf |
| apply_horizontal_diff_at_large_dt: bool = True # lhdiff_rcf | |
| apply_horizontal_diffusion_at_large_dt: bool = True | |
| """lhdiff_rcf in ICON.""" |
There was a problem hiding this comment.
Good point. I have replaced them with full name.
| 5 # ndyn_substeps is not a constant in ICON, it may be modified in restart runs | ||
| ) | ||
| apply_horizontal_diff_at_large_dt: bool = True # lhdiff_rcf | ||
| apply_extra_diffusion_for_largeCFL: bool = True # lextra_diffu |
There was a problem hiding this comment.
same here for the comment
| apply_extra_diffusion_for_largeCFL: bool = True # lextra_diffu | |
| apply_extra_diffusion_for_largeCFL: bool = True #: lextra_diffu |
| @@ -568,17 +568,17 @@ def run_predictor_step( | |||
| else: | |||
| lvn_only = False | |||
|
|
|||
There was a problem hiding this comment.
I still believe this if-condition is very obfuscated. (I know you did not change it...) but I always wondered what is the value of the lvn_only if l_init==False and l_recompute == False? I think it is not defined, and that is probably just ok because for some reason it never happens.
Furthermore isn't this the same:
| if l_init: | |
| lvn_only = False | |
| else if l_recompute and self.config.itime_scheme == 4: | |
| lvn_only = True |
There was a problem hiding this comment.
From my understanding, the outer if (l_init or l_recompute)) determines whether velocity_advection.run_predictor_step will be run. And lvn_only is only needed in velocity_advection.run_predictor_step. So, when l_init==False and l_recompute==False, the value of lvn_only does not matter. I remember Anurag said that we will possibly switch to itime_scheme=6 in the future and we can completely remove this weird lvn_only. I suggest we just leave it as the current state as a temporary solution.
There was a problem hiding this comment.
Hm, still don't like it. The condition is just very hard to read.
model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/nh_solve/solve_nonhydro.py
Show resolved
Hide resolved
model/driver/tests/test_timeloop.py
Outdated
| sp = timeloop_nonhydro_savepoint_init | ||
| nonhydro_params = NonHydrostaticParams(nonhydro_config) | ||
| sp_v = timeloop_velocity_savepoint_init | ||
| grid = SimpleGrid() |
There was a problem hiding this comment.
I think you can already delete this (SimpleGrid). All the other stuff, we will slowly improve over this cycle with the cleanup project, I hope
There was a problem hiding this comment.
Okay. I will now change it to ICON grid and we can remove them in the future after cleaning up.
| @@ -78,116 +85,241 @@ def _copy_diagnostic_and_prognostics( | |||
| _identity_c_k(theta_v_new, out=theta_v) | |||
There was a problem hiding this comment.
You are not calling that _copy_diagnostic_and_prognostic any more and indeed there should be no need anymore since you call the time integration. Can you delete it?
|
|
||
| self._l_init: bool = apply_initial_stabilization | ||
|
|
||
| self._now: int = 0 # TODO: move to PrognosticState |
There was a problem hiding this comment.
yes, I think it would be nice to keep the internals of the time - swapping somehow in the prognostic state, as we discussed. Latest, when not all fields inside the state change at the same time from now -> next, we should reconsider it.
|
cscs-ci run |
…ion was wrongly set to true after the review
|
cscs-ci run |
|
launch jenkins spack |
|
cscs-ci run |
|
launch jenkins spack |
| @@ -568,17 +568,17 @@ def run_predictor_step( | |||
| else: | |||
| lvn_only = False | |||
|
|
|||
There was a problem hiding this comment.
Hm, still don't like it. The condition is just very hard to read.
model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/nh_solve/solve_nonhydro.py
Show resolved
Hide resolved
| @@ -381,11 +385,13 @@ def test_nonhydro_predictor_step( | |||
| ) | |||
|
|
|||
| # stencil 35 | |||
| # TODO: first few levels are not computed in the test, please add a bound | |||
There was a problem hiding this comment.
I am always a bit confused by comments like that? does it run or not? Why not directly add the bound
There was a problem hiding this comment.
I missed that. I have added the bound accordingly. I also added a bound for CellDim w_concorr_c (stencil 39, 40) and z_flxdiv_theta (stencil 41) as I founded that those stencils are executed with these bounds. Please also have a look.
| else: | ||
| (model_run_config, diffusion_config, dycore_config) = _default_config(n_time_steps) | ||
| log.info("Warning: Experiment name is not specified, default configuration is used.") |
There was a problem hiding this comment.
Why not use a real warning instead?
| log.info("Warning: Experiment name is not specified, default configuration is used.") | |
| log.warning("Experiment name is not specified, default configuration is used.") |
| from icon4py.model.driver.icon_configuration import IconRunConfig | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
Can you add #TODO here that should reuse the code from the diffusion tests? I have restructured this in order to allow configuration for the Aquaplanet experiment, maybe that will make it easier once we bring this together.
There was a problem hiding this comment.
I have added a #TODO statement to reuse those pytest fixtures in the diffusion test.
| self._substep_timestep: float = float(self.run_config.dtime / self._n_substeps_var) | ||
|
|
||
| # check validity of configurations | ||
| self._validate() |
There was a problem hiding this comment.
if you rename this function to _validate_config then you don't need the extra comment.
| for t in range(self.config.n_time_steps): | ||
| log.info(f"run timestep : {t}") | ||
| timer = Timer(self._full_name(self._integrate_one_time_step)) | ||
| for i_time_step in range(self._n_time_steps): |
There was a problem hiding this comment.
why not just call it time_step this is some very akward old Fortran habit to call stuff ixxx and lxx to indicate the type.
There was a problem hiding this comment.
Good suggestion. I have removed the i in i_time_step. I also changed the idyn_timestep to dyn_substep in substepping.
|
|
||
| # TODO (Chia Rui): compute airmass for prognostic_state here | ||
|
|
||
| l_recompute = True |
There was a problem hiding this comment.
same here for the variables: why not just call them recompute, clean_mflx, dyn_timestep etc. If you want to indicate that it is a flag then I would rather choose do_recompute because
if do_recompute:
foo()
reads better than
if l_recompute:
foo()
There was a problem hiding this comment.
Good suggestion. I have systematically changed them to do_recompute, do_clean_mflx, and do_prep_adv which clearly shows their purposes.
|
Can you update the readme? |
…step in solve_nonhydro test_run_solve_nonhydro_single_step and test_run_solve_nonhydro_multi_step, update README.md in driver.
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests In case your change might affect downstream icon-exclaim, please consider running
For more detailed information please look at CI in the EXCLAIM universe. |
|
I have updated the README file and my test for the dycore driver with regards to the recent update of GT4Py and |
|
cscs-ci run |
|
launch jenkins spack |
|
cscs-ci run |
|
launch jenkins spack |
Constructed a simple dycore driver that performs time integration (a timeloop) with diffusion and solve_nonhydro components in reference to mo_nh_stepping.f90. Tracer advection and IO are not included. This will make the path for Jablonowski-Williamson test in the next phase by adding analytic initial conditions and a simple IO infrastructure.