-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add restart option when the SLM is coupled to MALI #99
Add restart option when the SLM is coupled to MALI #99
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.
@hollyhan , thanks for adding the restart capability for MALI when running with the SLM and for documenting your testing so thoroughly! I haven't done my own testing yet, but I have a couple questions before I do, so I'm submitting an initial review for now.
! find the right slmTimeStep | ||
call mpas_pool_get_config(liConfigs,'config_slm_coupling_interval', config_slm_coupling_interval) | ||
read(config_slm_coupling_interval(1:4),*) slm_coupling_interval | ||
call mpas_pool_get_config(liConfigs,'config_slm_timestep_restart', config_slm_timestep_restart) |
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.
So is the user required to manually set config_slm_timestep_restart
before doing a restart? What purpose does this serve if the code is able to calculate slmTimeStep_restart
on its own? I would prefer to not require an additional namelist change to do a restart with the SLM if it's not necessary. I think it's fair to assume that a user hasn't has not messed around with any output files before a restart.
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.
@matthewhoffman
The purpose is to make sure the restart time of MALI (taken either from config_start_time
or restart_timestamp
is setup correctly) is set correctly such that the restart year is a correct multiple of the year from config_slm_coupling_interval
. Note that slmTimeStep_restart
is calculated using currYear
, which comes from MALI. This wouldn't be needed if I'd used the mpas alarm functionality and make it automatic in figuring out what the proper restart year should be for the SLM, which could be done in the next PR at some point.
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.
I'm still confused. If you are checking to make sure these are identical, and then using the value if they are identical, can't we just use the internal calculation and not require the user to set this?
We could instead add a check that the slm coupling interval matches the restart interval. That is the requirement you are worried about, if I understand this correctly.
Thanks for the initial review, @matthewhoffman! |
This PR updates handling of timesteps between MALI and the SLM in a few ways: * switch config_slm_coupling_interval to be an integer in years because we only allow integer year values * On init, check that config_adaptive_timestep_force_interval divides evenly into config_slm_coupling_interval * On init, check that restart interval is an even multiple of config_slm_coupling_interval * On a restart, calculate which SLM time level to use based on the elapsed time from the start of the original simulation and config_slm_coupling_interval and make sure these divide cleanly
Also add missing =>next pointer assignment to keep code from hanging
Trying to cast intervals into dateTimeStrings did not work.
@hollyhan , I've pushed a handful of commits that revamp the logic for checking if the coupling interval is valid and for calculating the SLM time level on a restart without needing the user to do anything. I've tested that when I make things inconsistent it throws the appropriate errors, but it would be great if you could do your more thorough cold start and restart tests. I did confirm that I could get a restart running without error, and I think it is using the correct time level, but I did not compare the restart run results against a full cold start. |
@matthewhoffman, thanks for making this PR complete! It makes it a lot easier for the users this way to not have to assign a restart timestep number for the SLM. I've tested the new changes on both cold start and a restart against the full restart using the Zero difference in the gravitational field change ( Restart test: restart year 2040 (simulation start year 2015) Note that I also checked other fields such as predicted topography, also yield no difference compared to the cold start run. |
@@ -121,7 +121,7 @@ subroutine li_bedtopo_init(domain, err) | |||
|
|||
call mpas_pool_get_config(liConfigs, 'config_uplift_method', config_uplift_method) | |||
if (trim(config_uplift_method)=='sealevelmodel') then | |||
! initialize the 1D sea-level model | |||
! initialize the 1D sea-level model if fresh start |
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.
I don't think this change makes sense, because this call is happening whether this is a restart or cold start.
! initialize the 1D sea-level model if fresh start | |
! initialize the 1D sea-level model |
@@ -1197,7 +1198,7 @@ subroutine li_simulation_clock_init(core_clock, configs, ierr) | |||
|
|||
! Set up the coupling time interval if MALI is coupled to sea-level model | |||
if (trim(config_uplift_method) == "sealevelmodel") then | |||
call mpas_set_timeInterval(slm_coupling_interval, timeString=config_slm_coupling_interval, ierr=err_tmp) | |||
call mpas_set_timeInterval(slm_coupling_interval, YY=config_slm_coupling_interval, ierr=err_tmp) | |||
ierr = ior(ierr,err_tmp) | |||
call mpas_add_clock_alarm(core_clock, 'slmCouplingInterval', alarmTime=startTime, & |
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.
If we replace startTime
here with the mpas_time_type variable corresponding to the variable simulationStartTime
, I think we would be able to do restarts in the middle of coupling intervals. Note startTime
here is not simulationStartTime
, despite the similar name. simulationStartTime
is the start of the original simulation (i.e. the original cold start on a restart), whereas startTime
here is the start of this model execution (i.e. the restart time on a restart).
This commit changes how restarts are handled when the SLM is active to allow MALI to be restarted at any arbitrary restart interval and have the SLM restart correctly. This is done by changing the SLM coupling alarm to be based off of the original simulationStartTime (instead of the start time of the current execution). This required moving the creation of the coupling alarm to later in initialization so that the variable simulationStartTime is available. With this change, it was also necessary to change the way the SLM time level is calculated on a restart to take the floor of the elapsed time divided by the coupling interval, rather than requiring that there be no remainder. This adds a little fragility because there is no way to double check that is the correct SLM time level, but if this is set up correctly, it should be handled properly. Finally, as part of these changes, I also removed the check on init that the coupling interval divides evenly into the restart interval, because that's no longer a requirement. That's sort of too bad, because it was a lot of work to figure out how to make that check! But it's nicer to not have that restriction.
This doesn't serve any internal purpose, but it could help a user detect an error in their configuration.
err = ior(err, err_tmp) | ||
if (err == 0) then | ||
call mpas_log_write("Calling the SLM. SLM timestep $i", intArgs=(/slmTimeStep/)) | ||
call slmodel_solve(slmTimeStep, domain) |
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.
@hollyhan , do you know if this call to slmodel_solve
on init on a restart is necessary? The time level on init is a time level that was already fully run on the previous run, so I don't think we need to run the SLM again. It wouldn't hurt, but I think it can be skipped. Unless you are aware of a reason it is necessary.
To get restarts to work when using restart intervals shorter than the coupling interval, this call would need to be deleted or only made if the restart time happened to coincide with the coupling interval.
It's not needed, and if the restart time is not a coupling interval, it will make the SLM get out of sync.
TestingI ran a circular ice-sheet test that @hollyhan set up with and without restarts. It has restart output at 1 yr intervals and a SLM coupling interval of 5 yr. Simulation starts in 2015 and ends in 2055. Three experiments were done:
This configuration has a prescribed ice thickness time series, but bedTopography is updated from the SLM, so the VAF globalStat will be affected by the SLM solutions. This plot show the final few annual time levels of all three runs. Marker styles were chosen to ensure the values for all three runs could be seen. All three runs have the same results, confirming that restarts are working correctly. I also confirmed with ncdump that the values all match to machine precision. Model runs are on Chicoma at |
@matthewhoffman , thanks for the new test result! I've also confirmed the changes work for me as well. |
compass full_integration suite passes |
Update the namelist file to reflect the changes made in the recent PR (MALI-Dev/E3SM#99) Also change the simulation end year to 2031 as required by ISMIP6-2300
Update the namelist file to reflect the changes made in the recent PR (MALI-Dev/E3SM#99) Also change the simulation end year to 2031 as required by ISMIP6-2300
Previously, when the regional sea-level prediction capability was added to MALI (#21), the restart config option for the sea-level model was not added. This led the sea-level model to get initialized to Timestep zero when coupled MALI-SLM simulations are being restarted, forgetting about the ice loading changes and associated viscoelastic solid earth deformation that happened in the timesteps prior to current model time. This PR fixes the problem by allowing the sea-level model to resume where it was left off. Note in parallel to this PR, the version of the SLM needs to incorporate the changes made in the following accompanying PR (MALI-Dev/1DSeaLevelModel_FWTW#9)
Testing results (on Chicoma)
Test directory:
/lustre/scratch4/turquoise/hollyhan/testPR_restart_coupled_simulations/landice/slm/circular_icesheet_test/mali20km_slm512
This test utilizes the
compass/landice/slm/circicesheet
test group (hollyhan:hollyhan/compass/landice/slm_circ_icesheet) in setting a coupled MALI-SLM simulation on MALI 20km rectangular grid with cylindrical ice sheet retreating 2km radius per year and SLM SH512 resolution.The following config options can be used to setup the circular ice sheet test case on chicoma:
Version of SLM used: [SHA 286336ddd1ea884f9c397729511853e9f6fd276a added on top of the head of
origin/main
(SHA 42478dd3c3eefaeeb71d45dc7c8db7f42795519b)]Temporal resolution of the SMB forcing applied to MALI: 1 yr
Coupling interval between MALI and the SLM: 5 yrs
Simulation start/end year: 2015/2045
Coupled simulation length: 40 yrs
Total number of SLM step in full simulation: 8 (40 yrs divided by 5 yrs)
Test simulations setup
**Simulation 1: Cold-start simulations
Simulation 1-1:Benchmark run where MALI runs from a cold start
Version of MALI used: [SHA 859158a]
(
/lustre/scratch4/turquoise/hollyhan/testPR_restart_coupled_simulations/landice/slm/circular_icesheet_test/mali20km_slm512/run_model_restartPR_coldStart/folder_maliHead_3079c717b3/OUTPUT_SLM
)Plot showing the changes in gravitational potential over 40 simulation years:
Simulation 1-2: Cold start with the head of this PR [SHA 35bed8a]
(
/lustre/scratch4/turquoise/hollyhan/testPR_restart_coupled_simulations/landice/slm/circular_icesheet_test/mali20km_slm512/run_model_restartPR_coldStart/folder_maliHead_35bed8a9e6/OUTPUT_SLM
)Difference in deltaG field (change in gravity potential over 40 years): Calculated by subtracting the result from the benchmark simulation (Simulation 1-1) from the result from this simulation (Sim. 1-2).
Simulation 2: Coupled simulation restarts from simulation year 30 (
/lustre/scratch4/turquoise/hollyhan/testPR_restart_coupled_simulations/landice/slm/circular_icesheet_test/mali20km_slm512/run_model_restartPR_reStart
)Difference in the fields above to the same field calculated by the benchmark simulation (Simulation #1-1) shows zero difference:
(The difference file is located here:
/lustre/scratch4/turquoise/hollyhan/testPR_restart_coupled_simulations/landice/slm/circular_icesheet_test/mali20km_slm512/run_model_restartPR_reStart/OUTPUT_SLM_slmRestartYear2045_maliRestartYear2045/diff_G8_to_originDevelop.nc
and
log.landice.0000.out
file dies with the following message:The log files can be found here:
/lustre/scratch4/turquoise/hollyhan/testPR_restart_coupled_simulations/landice/slm/circular_icesheet_test/mali20km_slm512/run_model_restartPR_reStart/output_logfiles_slmRestartYear2040_maliRestartYear2045