Closed glwagner closed 3 years ago
Agree that moving precomputations!(model)
to the start of time_step!
makes a lot of sense, but I would probably move the iteration == 0
call to the start of time_step!
instead of the start of run!
as there may be use cases where we want to time_step!
manually and I'm sure someone will forget to call precomputations!(model)
leading to head-scratching and bugs.
This is also a nice change as it changes the time-stepping algorithm but we shouldn't need to regenerate the regression data.
This is also a nice change as it changes the time-stepping algorithm but we shouldn't need to regenerate the regression data.
We omit halos from the regression test, right?
If we make this change, we may want to update the regression tests to include halos, pressures, and diffusivities (though it wouldn't be necessary, it might be wise)...
True, those would be good additions to the regression tests.
But I might wait until we truly need to regenerate regression data as regenerating regression data adds to the size of the git repo that can only be recovered by rewriting git history. We did this once but probably don't want to do again as it pollutes git history and we may need to run tests on old versions(?).
I think regression files currently take up ~17.1 MiB of space in the git repo while a fresh clone of the repo is ~43 MiB (images and convergence plots probably take up several MiB).
Here's a listing of all files in git history over 300 KiB (command from https://stackoverflow.com/a/42544963):
018186272590 328KiB test/data_rayleigh_benard_regression_000001100.jld
19db949aaae8 328KiB test/data_rayleigh_benard_regression_000001000.jld
424080660c53 328KiB test/data_rayleigh_benard_regression_000001000.jld
a7e1d690d6b5 328KiB test/data_rayleigh_benard_regression_000001100.jld
72744372e5c4 361KiB test/regression_tests/data/thermal_bubble_regression.nc
4ce9699176ee 363KiB test/deep_convection_regression_10.nc
c15f95e2bf3a 364KiB test/regression_tests/data/thermal_bubble_regression.nc
6f28044e3b56 366KiB docs/src/verification/convergence_plots/gaussian_advection_diffusion_error_convergence.png
194fdf47099b 392KiB docs/src/verification/convergence_plots/gaussian_advection_diffusion_error_convergence.png
2f9d5e8650d7 420KiB docs/src/verification/convergence_plots/cosine_advection_diffusion_error_convergence.png
db8f742e7c95 446KiB docs/src/verification/convergence_plots/cosine_advection_diffusion_error_convergence.png
0de880b2b97b 468KiB docs/src/verification/plots_stratified_couette_flow_stratified_couette_flow_velocity_temperature_slices.png
d277a4e5393b 650KiB test/regression_tests/data/data_rayleigh_benard_regression.jld2
b125bc6f8e9d 709KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_10000.jld2
f5c1a7736324 709KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_10010.jld2
0b493fa7dd14 709KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_10000.jld2
ad020f12370b 709KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_10010.jld2
9879b0da29c0 709KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_10010.jld2
c170cc80cd64 709KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_10000.jld2
a5a23cbaaace 709KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_10000.jld2
b62c38aea554 709KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_10010.jld2
9765742b042b 713KiB test/regression_tests/data/rayleigh_benard_iteration1000.jld2
d6932dc59613 713KiB test/regression_tests/data/rayleigh_benard_iteration1100.jld2
5b796cdfdf8e 718KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_iteration10010.jld2
ba4645921310 718KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_iteration10000.jld2
3519eeb0dea0 718KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_iteration10010.jld2
fbf720bf84dc 718KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_iteration10000.jld2
51891abf2cd1 719KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_iteration10010.jld2
c48525b35c1b 719KiB test/regression_tests/data/ocean_large_eddy_simulation_SmagorinskyLilly_iteration10000.jld2
41f8e56c345f 719KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_iteration10000.jld2
a7a57fa8fdc7 719KiB test/regression_tests/data/ocean_large_eddy_simulation_VerstappenAnisotropicMinimumDissipation_iteration10010.jld2
0ee7298c84ad 731KiB test/thermal_bubble_golden_master_model_checkpoint_10.jld
bddab0c2f590 924KiB test/regression_tests/data/data_rayleigh_benard_regression.jld2
937939cc1ef2 990KiB docs/src/verification/convergence_plots/gaussian_advection_diffusion_solutions.png
841a7461932f 1.0MiB docs/src/verification/convergence_plots/gaussian_advection_diffusion_solutions.png
061ab36b8d44 1.3MiB docs/src/verification/convergence_plots/cosine_advection_diffusion_solutions.png
2f48fac8a7f5 1.4MiB paper/free_convection_and_baroclinic_instability.png
7ef3d2c84f36 1.4MiB docs/src/verification/convergence_plots/cosine_advection_diffusion_solutions.png
Resolved by #1083
I think we want to consider calling
precomputations!(model)
(and perhaps changing its name) after a time-step is complete, rather than before.The reason to do this is that it ensures all fields are entirely up-to-date and synchronized between time-steps. Currently, for example, field halo regions, hydrostatic pressure, and diffusivities are a "step behind" model.velocities and model.tracers. This is a problem for some diagnostics; for example, turbulent kinetic energy dissipation cannot be correctly calculated during output. Correct halo regions also cannot be saved. Having the entire model state synced at the end of a time-step is thus important for correct output. There's no extra computational cost to this procedure. However, we will have to execute one special call to
precomputations!(model)
insiderun!(simulation)
to initialize a time-stepping loop. I think this price is worth it.This change will mean that all aspects of the model state are "untouchable" between time-steps. In other words, one cannot change halos or the pressure fields at whim. I think this is ok, since this is already true for the majority of the model data (interior parts of the model velocities and tracer fields).
This issue is entangled with #1063 since we cannot resolve this issue until we have standard output that does not change a field's halo regions.