-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cicecore: correct initial condition metadata #818
cicecore: correct initial condition metadata #818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Just to clarify, is the history ic file data identical compared to baseline? Did just the filename and some attributes change? Or is the file written at a different part of the initialization sequence or even prior to advancing now? |
Thanks for your questions, it made me take a closer look at the standalone code. The data is now written prior to advancing the calendar by calling The diff below shows the whole context in between the addition and the deletion for the standalone driver: ! Initialize shortwave components using swdn from previous timestep
! if restarting. These components will be scaled to current forcing
! in prep_radiation.
if (trim(runtype) == 'continue' .or. restart) &
call init_shortwave ! initialize radiative transfer
+ if (write_ic) call accum_hist(dt) ! write initial conditions
+
! tcraig, use advance_timestep here
! istep = istep + 1 ! update time step counters
! istep1 = istep1 + 1
! time = time + dt ! determine the time and date
! call calendar(time) ! at the end of the first timestep
call advance_timestep()
!--------------------------------------------------------------------
! coupler communication or forcing data initialization
!--------------------------------------------------------------------
call init_forcing_atmo ! initialize atmospheric forcing (standalone)
if (tr_fsd .and. wave_spec) call get_wave_spec ! wave spectrum in ice
call get_forcing_atmo ! atmospheric forcing from data
call get_forcing_ocn(dt) ! ocean forcing from data
! snow aging lookup table initialization
if (tr_snow) then ! advanced snow physics
call icepack_init_snow()
call icepack_warnings_flush(nu_diag)
if (icepack_warnings_aborted()) call abort_ice(error_message=subname, &
file=__FILE__, line=__LINE__)
if (snw_aging_table(1:4) /= 'test') then
call init_snowtable()
endif
endif
! isotopes
if (tr_iso) call fiso_default ! default values
! aerosols
! if (tr_aero) call faero_data ! data file
! if (tr_zaero) call fzaero_data ! data file (gx1)
if (tr_aero .or. tr_zaero) call faero_default ! default values
if (skl_bgc .or. z_tracers) call get_forcing_bgc ! biogeochemistry
if (z_tracers) call get_atm_bgc ! biogeochemistry
if (runtype == 'initial' .and. .not. restart) &
call init_shortwave ! initialize radiative transfer using current swdn
call init_flux_atm ! initialize atmosphere fluxes sent to coupler
call init_flux_ocn ! initialize ocean fluxes sent to coupler
- if (write_ic) call accum_hist(dt) ! write initial conditions
-
if (my_task == master_task) then
call ice_memusage_print(nu_diag,subname//':end')
endif
end subroutine cice_init So, I think that since the forcing is read after the initial condition is written (with my changes), if any output variable that correspond to forcing fields (e.g. It's not the case in our NEMO driver since all forcing is passed to CICE by NEMO. So an alternative, instead of moving the Apart from that, the changes are mostly in attributes, with this change, in the initial condition :
|
I've just checked that's indeed the case. |
So, with this diff instead, the filename and attributes are OK (time=0), etc., and the data stays the same as it is now: diff --git a/./cicecore/drivers/standalone/cice/CICE_InitMod.F90 b/./cicecore/drivers/standalone/cice/CICE_InitMod.F90
index 8de05a1..c94b431 100644
--- a/./cicecore/drivers/standalone/cice/CICE_InitMod.F90
+++ b/./cicecore/drivers/standalone/cice/CICE_InitMod.F90
@@ -192,27 +192,20 @@ subroutine cice_init
if (tr_aero .or. tr_zaero) call faero_optics !initialize aerosol optical
!property tables
! Initialize shortwave components using swdn from previous timestep
! if restarting. These components will be scaled to current forcing
! in prep_radiation.
if (trim(runtype) == 'continue' .or. restart) &
call init_shortwave ! initialize radiative transfer
-! tcraig, use advance_timestep here
-! istep = istep + 1 ! update time step counters
-! istep1 = istep1 + 1
-! time = time + dt ! determine the time and date
-! call calendar(time) ! at the end of the first timestep
- call advance_timestep()
-
!--------------------------------------------------------------------
! coupler communication or forcing data initialization
!--------------------------------------------------------------------
call init_forcing_atmo ! initialize atmospheric forcing (standalone)
if (tr_fsd .and. wave_spec) call get_wave_spec ! wave spectrum in ice
call get_forcing_atmo ! atmospheric forcing from data
call get_forcing_ocn(dt) ! ocean forcing from data
@@ -238,20 +231,28 @@ subroutine cice_init
if (z_tracers) call get_atm_bgc ! biogeochemistry
if (runtype == 'initial' .and. .not. restart) &
call init_shortwave ! initialize radiative transfer using current swdn
call init_flux_atm ! initialize atmosphere fluxes sent to coupler
call init_flux_ocn ! initialize ocean fluxes sent to coupler
if (write_ic) call accum_hist(dt) ! write initial conditions
+! tcraig, use advance_timestep here
+! istep = istep + 1 ! update time step counters
+! istep1 = istep1 + 1
+! time = time + dt ! determine the time and date
+! call calendar(time) ! at the end of the first timestep
+ call advance_timestep()
+
+
if (my_task == master_task) then
call ice_memusage_print(nu_diag,subname//':end')
endif
end subroutine cice_init I think this is a better approach, Let me know what you think and I can update the PR. |
Thanks @phil-blain. I'm a little surprised that shifting the call of advance timestep later in the sequence doesn't change the model results. The model time will be different when some of those init methods are called. It could be they are just setting up the run, not actually initializing the data based on model time. In that case, that's great. But we should probably run a full test suite just to make sure the answers don't change. I can help do that if you like, just let me know. |
Yes, I was planning on running a full suite. I'll fire that up right now. |
Thanks for fixing this, @phil-blain! When writing of the initial condition was added many years ago, the whole initialization process was pretty complicated and different in various coupled models, and I was mainly interested in seeing the initial ice state, not so much the forcing. So this is an improvement. I definitely think it should be tested not only in standalone mode but also the various coupled configurations, to make sure they're all BFB in their solutions, if not the initial condition file output itself. |
OK, so as hinted yesterday during our call, moving the call to So I see two options:
Let me know what you think. |
Hi Philippe,
My vote is option 2 to get the initial condition written with the initial time, even if forcing is zero. It is more clear to me that it is an initial condition if the time matches the restart time.
Thank you,
David
[edited to clean up email]
|
There is also option 3:
|
I think we don't want to change model answers unless we do a careful refactor of the standalone sequencing with respect to forcing, time advancement, and model timestepping. I'm open to doing that, but it's a bigger effort. I think option 2 is a good compromise at this point, but am open. I think @eclare108213 should weigh in too. |
Is option 2 what is currently coded? This looks okay to me. |
Thanks @eclare108213. Yes, option 2 is what is currently in this PR. I'll run a full suite just to be extra careful, I don't expect answers to change. |
When writing averaged history outputs (hist_avg=.true.), this setting also affects the initial condition. Even if the actual data variables written to the initial condition are not averaged (they are taken more or less directly from the restart or the hard-coded defaults, modulo aggregation over categories), their attributes ('cell_method' and 'time_rep') imply they are averaged, and the 'bound' attribute of the 'time' variable refers to the 'time_bounds' variable. Make the metadata of the initial condition more correct by: - not writing the 'time_bounds' variable (and the corresponding 'd2' dimension) - not writing the 'bounds' attribute of the 'time' variable - not writing the 'cell_method' attributes of each variable - writing the 'time_rep' attribute of each variable as 'instantaneous' instead of 'averaged'. Do this by checking 'write_ic' at all places where we check for the value of 'hist_avg' to write the above variables and attributes in each of the 3 IO backends (binary, netcdf, pio2).
…l time In CICE_InitMod::cice_init, we call ice_calendar::advance_timestep before writing the initial condition, such that the 'time' variable in the initial condition is not zero; it has a value of 1*dt (the model time step). The initial condition filename also reflects this, since 'msec' (model seconds) also has a value of 1*dt and is used in ice_history_shared::construct_filename. This leads to the initial condition filename not corresponding to the model initialization date/time but rather 1*dt later. Since we call 'accum_hist' after initializing the forcing, any forcing field written to the initial condition has values corresponding to msec=dt, whereas the ice state corresponds to msec=0, leading to an inconsistency. Fix that by calling 'accum_hist' to write the initial condition _before_ calling 'advance_timestep'. Since we now call 'accum_hist' before initializing the forcing, any forcing field written to the initial condition have its default, hard-coded value, instead of its value at time=dt. An improvement would be to read the forcing at time=dt, write the initial condition, advance the time step, and read the forcing again, but let's not complicate things too much for now.
25b7c2b
to
d72cdf9
Compare
Base suite is b4b:
I've just pushed an updated version, only commit messages are changed, range-diff:
|
* ice_history_write: fix initial condition metadata under 'hist_avg' When writing averaged history outputs (hist_avg=.true.), this setting also affects the initial condition. Even if the actual data variables written to the initial condition are not averaged (they are taken more or less directly from the restart or the hard-coded defaults, modulo aggregation over categories), their attributes ('cell_method' and 'time_rep') imply they are averaged, and the 'bound' attribute of the 'time' variable refers to the 'time_bounds' variable. Make the metadata of the initial condition more correct by: - not writing the 'time_bounds' variable (and the corresponding 'd2' dimension) - not writing the 'bounds' attribute of the 'time' variable - not writing the 'cell_method' attributes of each variable - writing the 'time_rep' attribute of each variable as 'instantaneous' instead of 'averaged'. Do this by checking 'write_ic' at all places where we check for the value of 'hist_avg' to write the above variables and attributes in each of the 3 IO backends (binary, netcdf, pio2). * drivers/{nemo_concepts,standalone}: write initial condition at initial time In CICE_InitMod::cice_init, we call ice_calendar::advance_timestep before writing the initial condition, such that the 'time' variable in the initial condition is not zero; it has a value of 1*dt (the model time step). The initial condition filename also reflects this, since 'msec' (model seconds) also has a value of 1*dt and is used in ice_history_shared::construct_filename. This leads to the initial condition filename not corresponding to the model initialization date/time but rather 1*dt later. Since we call 'accum_hist' after initializing the forcing, any forcing field written to the initial condition has values corresponding to msec=dt, whereas the ice state corresponds to msec=0, leading to an inconsistency. Fix that by calling 'accum_hist' to write the initial condition _before_ calling 'advance_timestep'. Since we now call 'accum_hist' before initializing the forcing, any forcing field written to the initial condition have its default, hard-coded value, instead of its value at time=dt. An improvement would be to read the forcing at time=dt, write the initial condition, advance the time step, and read the forcing again, but let's not complicate things too much for now. (cherry picked from commit 9a55ad9) Cherry-pick notes: There were some conflicts in io_{netcdf,pio2}/ice_history_write.F90 due to 26d917a (Fix QC test, fix bug in history time axis, fix history output averaging for timestep output (CICE-Consortium#624), 2021-08-19), since that commit enabled averaging over multiple time steps and thus removed the assumption that histfreq(ns) = '1' means hist_avg = .false.. I also had to add additional changes to 'ice_history_write' in the conditions that are checked before writing the NetCDF attributes since we do not yet have the 'ice_write_hist_attrs' subroutine added in 078aab4 (Merge cgridDEV branch including C grid implementation and other fixes (CICE-Consortium#715), 2022-05-10). Closes: https://gitlab.science.gc.ca/concepts/CICE/issues/19
* ice_history_write: fix initial condition metadata under 'hist_avg' When writing averaged history outputs (hist_avg=.true.), this setting also affects the initial condition. Even if the actual data variables written to the initial condition are not averaged (they are taken more or less directly from the restart or the hard-coded defaults, modulo aggregation over categories), their attributes ('cell_method' and 'time_rep') imply they are averaged, and the 'bound' attribute of the 'time' variable refers to the 'time_bounds' variable. Make the metadata of the initial condition more correct by: - not writing the 'time_bounds' variable (and the corresponding 'd2' dimension) - not writing the 'bounds' attribute of the 'time' variable - not writing the 'cell_method' attributes of each variable - writing the 'time_rep' attribute of each variable as 'instantaneous' instead of 'averaged'. Do this by checking 'write_ic' at all places where we check for the value of 'hist_avg' to write the above variables and attributes in each of the 3 IO backends (binary, netcdf, pio2). * drivers/{nemo_concepts,standalone}: write initial condition at initial time In CICE_InitMod::cice_init, we call ice_calendar::advance_timestep before writing the initial condition, such that the 'time' variable in the initial condition is not zero; it has a value of 1*dt (the model time step). The initial condition filename also reflects this, since 'msec' (model seconds) also has a value of 1*dt and is used in ice_history_shared::construct_filename. This leads to the initial condition filename not corresponding to the model initialization date/time but rather 1*dt later. Since we call 'accum_hist' after initializing the forcing, any forcing field written to the initial condition has values corresponding to msec=dt, whereas the ice state corresponds to msec=0, leading to an inconsistency. Fix that by calling 'accum_hist' to write the initial condition _before_ calling 'advance_timestep'. Since we now call 'accum_hist' before initializing the forcing, any forcing field written to the initial condition have its default, hard-coded value, instead of its value at time=dt. An improvement would be to read the forcing at time=dt, write the initial condition, advance the time step, and read the forcing again, but let's not complicate things too much for now. (cherry picked from commit 9a55ad9) Cherry-pick notes: There were some conflicts in io_{netcdf,pio2}/ice_history_write.F90 due to 26d917a (Fix QC test, fix bug in history time axis, fix history output averaging for timestep output (CICE-Consortium#624), 2021-08-19), since that commit enabled averaging over multiple time steps and thus removed the assumption that histfreq(ns) = '1' means hist_avg = .false.. I also had to add additional changes to 'ice_history_write' in the conditions that are checked before writing the NetCDF attributes since we do not yet have the 'ice_write_hist_attrs' subroutine added in 078aab4 (Merge cgridDEV branch including C grid implementation and other fixes (CICE-Consortium#715), 2022-05-10).
cicecore: correct initial condition metadata (CICE-Consortium#818) Closes #19
* ice_history_write: fix initial condition metadata under 'hist_avg' When writing averaged history outputs (hist_avg=.true.), this setting also affects the initial condition. Even if the actual data variables written to the initial condition are not averaged (they are taken more or less directly from the restart or the hard-coded defaults, modulo aggregation over categories), their attributes ('cell_method' and 'time_rep') imply they are averaged, and the 'bound' attribute of the 'time' variable refers to the 'time_bounds' variable. Make the metadata of the initial condition more correct by: - not writing the 'time_bounds' variable (and the corresponding 'd2' dimension) - not writing the 'bounds' attribute of the 'time' variable - not writing the 'cell_method' attributes of each variable - writing the 'time_rep' attribute of each variable as 'instantaneous' instead of 'averaged'. Do this by checking 'write_ic' at all places where we check for the value of 'hist_avg' to write the above variables and attributes in each of the 3 IO backends (binary, netcdf, pio2). * drivers/{nemo_concepts,standalone}: write initial condition at initial time In CICE_InitMod::cice_init, we call ice_calendar::advance_timestep before writing the initial condition, such that the 'time' variable in the initial condition is not zero; it has a value of 1*dt (the model time step). The initial condition filename also reflects this, since 'msec' (model seconds) also has a value of 1*dt and is used in ice_history_shared::construct_filename. This leads to the initial condition filename not corresponding to the model initialization date/time but rather 1*dt later. Since we call 'accum_hist' after initializing the forcing, any forcing field written to the initial condition has values corresponding to msec=dt, whereas the ice state corresponds to msec=0, leading to an inconsistency. Fix that by calling 'accum_hist' to write the initial condition _before_ calling 'advance_timestep'. Since we now call 'accum_hist' before initializing the forcing, any forcing field written to the initial condition have its default, hard-coded value, instead of its value at time=dt. An improvement would be to read the forcing at time=dt, write the initial condition, advance the time step, and read the forcing again, but let's not complicate things too much for now. (cherry picked from commit 9a55ad9) Cherry-pick notes: There were some conflicts in io_{netcdf,pio2}/ice_history_write.F90 due to 26d917a (Fix QC test, fix bug in history time axis, fix history output averaging for timestep output (CICE-Consortium#624), 2021-08-19), since that commit enabled averaging over multiple time steps and thus removed the assumption that histfreq(ns) = '1' means hist_avg = .false.. I also had to add additional changes to 'ice_history_write' in the conditions that are checked before writing the NetCDF attributes since we do not yet have the 'ice_write_hist_attrs' subroutine added in 078aab4 (Merge cgridDEV branch including C grid implementation and other fixes (CICE-Consortium#715), 2022-05-10).
cicecore: correct initial condition metadata (CICE-Consortium#818) Closes #19
* ice_history_write: fix initial condition metadata under 'hist_avg' When writing averaged history outputs (hist_avg=.true.), this setting also affects the initial condition. Even if the actual data variables written to the initial condition are not averaged (they are taken more or less directly from the restart or the hard-coded defaults, modulo aggregation over categories), their attributes ('cell_method' and 'time_rep') imply they are averaged, and the 'bound' attribute of the 'time' variable refers to the 'time_bounds' variable. Make the metadata of the initial condition more correct by: - not writing the 'time_bounds' variable (and the corresponding 'd2' dimension) - not writing the 'bounds' attribute of the 'time' variable - not writing the 'cell_method' attributes of each variable - writing the 'time_rep' attribute of each variable as 'instantaneous' instead of 'averaged'. Do this by checking 'write_ic' at all places where we check for the value of 'hist_avg' to write the above variables and attributes in each of the 3 IO backends (binary, netcdf, pio2). * drivers/{nemo_concepts,standalone}: write initial condition at initial time In CICE_InitMod::cice_init, we call ice_calendar::advance_timestep before writing the initial condition, such that the 'time' variable in the initial condition is not zero; it has a value of 1*dt (the model time step). The initial condition filename also reflects this, since 'msec' (model seconds) also has a value of 1*dt and is used in ice_history_shared::construct_filename. This leads to the initial condition filename not corresponding to the model initialization date/time but rather 1*dt later. Since we call 'accum_hist' after initializing the forcing, any forcing field written to the initial condition has values corresponding to msec=dt, whereas the ice state corresponds to msec=0, leading to an inconsistency. Fix that by calling 'accum_hist' to write the initial condition _before_ calling 'advance_timestep'. Since we now call 'accum_hist' before initializing the forcing, any forcing field written to the initial condition have its default, hard-coded value, instead of its value at time=dt. An improvement would be to read the forcing at time=dt, write the initial condition, advance the time step, and read the forcing again, but let's not complicate things too much for now. (cherry picked from commit 9a55ad9) Cherry-pick notes: There were some conflicts in io_{netcdf,pio2}/ice_history_write.F90 due to 26d917a (Fix QC test, fix bug in history time axis, fix history output averaging for timestep output (CICE-Consortium#624), 2021-08-19), since that commit enabled averaging over multiple time steps and thus removed the assumption that histfreq(ns) = '1' means hist_avg = .false.. I also had to add additional changes to 'ice_history_write' in the conditions that are checked before writing the NetCDF attributes since we do not yet have the 'ice_write_hist_attrs' subroutine added in 078aab4 (Merge cgridDEV branch including C grid implementation and other fixes (CICE-Consortium#715), 2022-05-10). Rebase onto CICE6.4.1 notes: There were some conflicts in io_{netcdf,pio2}/ice_history_write.F90 due to the same commits mentioned above. They are both included in CICE6.4.1, but the original cherry-picked commit (9a55ad9 (cicecore: correct initial condition metadata (CICE-Consortium#818), 2023-03-13)) is not. The correct resolution was thus to: - keep ours version for calls to ice_write_hist_attrs - cherry-pick from 9a55ad9 the changes to ice_write_hist_attrs - in io_pio2, change 'if (hist_avg)' to 'if (hist_avg .and. .not. write_ic)'
cicecore: correct initial condition metadata (CICE-Consortium#818) Closes #19
PR checklist
Make the initial condition metadata more correct by writing it at the initial time and not marking its variables as averaged.
I did not run the test suite, as this only changes
history
output and the our regression test compare the restarts, not the history output. I checked the following:time
netCDF variable in the initial condition has value zero and nobounds
attributetime_bounds
variable is not written in the initial conditiontime_rep = "instantaneous"
and nocell_method
attribute.cmp
since the time of the run is written as a global:history
attribute, but I used something liketo verify that the only difference is this global
history
attribute.I reported these two issues initially in #567 (comment):
I can remove the changes to the standalone driver if we do not agree on that, but in my opinion it does not break anything and makes the history output more correct so I felt it was better to just do it there also.
Full commit messages follow.