Update testing, in particular for diagnostics and decompositions #602
Conversation
- add alt06 test, ncat=7, kcatbound=3, nslyr=3 - add bigdiag test with lots of diagnostic output, debug_model=true, debug_blocks=true, etc - add spiralcenter decomp test - add gx1 weightfile decomp test - add gbox180 with spacecurve decomp test to trigger Peano and Cinco decompositions - rename boxdyn to boxnodyn - modify boxslotcyl, turn off tr_lvl and tr_pond_lvl, changes answers - clarify boxadv, boxnodyn, boxrestore, explicitly set shortwave=ccsm3, albedo_type=constant - cleanup spacecurve implementation including debug flagging, recode print to write, module private, add printcurve for gridsize of 30, remove qsort and partition subroutines. - remove CICE_RunMod.F90_debug and migrate high level debug checks to CICE_RunMod.F90 with new debug_model namelist. Also add debug_model_step to specify at what timestep to start writing debug_model output. debug_model writes data for the first point specified by lonpnt, latpnt. - move debug_ice routine into ice_diagnostics.F90 and out of CICE.F90 - remove writeout_finished_file from CICE_Finalize in several drivers - deprecate print_points_state via an ifdef - rename l_stop in ice_transport_driver.F90 to ckflag. This looked like l_stop logic because of the naming but it isn't. - fix issue of size of fcontopn_d and fsurfn_d when ncat greater than 6 in init_coupler_flux. Would go out of bounds, now initializes fcontopn and fsurfn to 6th value for any category greater than 6. - rename dbug variable name in ice_forcing.F90 to forcing_diag and change namelist variable name. - update ice_distributionGet output and implementation to support more general arguments - add ice_distributionGet test in init_domain_distribution when debug_blocks is true. - fix bug in create_distrb_rake for 1d rake. add a test that triggers the 1d rake. - update debug flag in ice_distribution.F90 leverage debug_blocks. - update documentation.
- Update calchk to check 100,000 years - Update sectrobin decomp test to check land block elimination
There was a problem hiding this comment.
Looks great, thanks @apcraig. I have some questions but nothing that keeps me from approving this as it is.
Does anyone (or any modeling group) use the space-filling curves distribution?
In the diff of ug_case_setting.rst, it looks like the first many (200?) lines of the file are italized but later ones are not. Is that a problem in the file itself, or just with this diff?
| @@ -29,14 +29,16 @@ | |||
| diagfreq = 24 | |||
| diag_type = 'stdout' | |||
| diag_file = 'ice_diag.d' | |||
| debug_model = .false. | |||
| debug_model_step = 999999999 | |||
There was a problem hiding this comment.
Would it be better to set debug_model_step to 0 so that it always starts printing immediately, as the default? Assuming it's ignored unless debug_model=T.
There was a problem hiding this comment.
It was 99999999 by default before, so I kept it that way. I'm happy to make the default 0, let me know if you prefer that.
There was a problem hiding this comment.
I think 0 would be better, since you have a flag to turn it off.
There was a problem hiding this comment.
Sounds good, I'll make this change in the next test coverage PR.
There was a problem hiding this comment.
I can do it in this PR too. I sort of trying to avoid doing some extra testing. Given this change is minor, nobody is going to use this feature at the moment, and this will be updated on the trunk in the next week (hopefully), I thought I'd just wait. I have three options
- make the change on this PR and rerun all or a subset of tests
- make the change on the PR and not test
- make the change in the next PR and carry out a full test suite (which I'd do anyway)
I was going for the 3rd option. I can do the 1st. I try to avoid doing the 2nd when changes occur in the source code or scripts as it's bad practice and risky.
There was a problem hiding this comment.
That's fine, whatever works best for you.
| @@ -23,3 +23,4 @@ Ktens = 0. | |||
| e_ratio = 2. | |||
| seabed_stress = .true. | |||
| use_bathymetry = .true. | |||
| l_mpond_fresh = .true. | |||
There was a problem hiding this comment.
If particular changes are addressing lines in the spreadsheet, could you add a note on the line that says which test script was changed, and how?
There was a problem hiding this comment.
This recommendation came from #437. #437 (comment). But it doesn't change answers which is a little concerning and something that we need to look into more probably.
There was a problem hiding this comment.
Here's what to look for: l_mpond_fresh might only be used for coupling, and the mixed layer model only calculates heat fluxes, not water/salt, so l_mpond_fresh might not be testable in our standalone configurations.
There was a problem hiding this comment.
So far as I can tell, l_mpond_fresh shows up in CICE_RunMod.F90 in the standalone model as
if (l_mpond_fresh) then
fpond(i,j,iblk) = fpond(i,j,iblk) * rhofresh/dt
fresh(i,j,iblk) = fresh(i,j,iblk) - fpond(i,j,iblk)
endif
I'm not sure what happens to fpond or fresh after that, but it doesn't seem to impact the solution. I guess I think that's OK. When we turn on options, it would be nice to see an impact, but if that's not possible, I still think it worthwhile to have a test that turns the flag on and at least tests the code in a technical sense.
| @@ -138,11 +145,11 @@ conflicts in module dependencies. | |||
|
|
|||
| `print\_points` (**ice\_in**) | |||
| If true, print numerous diagnostic quantities for two grid cells, | |||
| one near the north pole and one in the Weddell Sea. This utility | |||
| defined by `lonpnt` and `latpnt` in the namelist file. | |||
There was a problem hiding this comment.
I understand that this will be generalized so that the user alternatively can insert the indices directly, perhaps in a later PR.
There was a problem hiding this comment.
That is my plan. That feature will be added soon.
|
Hi @apcraig,
This should indeed be added to the gx1 input files, since it is used in the |
| if (debug_blocks .and. my_task == master_task) then | ||
| write(nu_diag,'(2a,3i8)') & |
There was a problem hiding this comment.
Hi @apcraig, I did not have time to review this PR before it was merged.
Here I notice that you prevented the block distribution from being outputed for all procs. Was this intended ? If we are debugging decompositions, I would expect we want output from all procs....
I was using debug_blocks just now to debug a segfault and was very confused as to why all blocks were listed on proc 1, and I dug up this PR by looking at the history of create_local_block_ids...
In our in-house CICE4 version, we had special initialization code in ice_flux::init_coupler_flux to initialize the 'fcondtopn_d' and 'fsurfn_d' arrays for ncat != 6. This was needed to avoid going out of bounds on the loop on ncat where these arrays are used below to initialize fcondtopn_f and fsurfn_f. This was fixed in CICE in 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26) by using the last (6th) value of the array for any higher category. This leads to 'fcondtopn_d' having different values in CICE6 for the last 4 categories compared to our in-house CICE4. To make side-by-side debugging of CICE6 and CICE4 easier, minimize the differences between the initialization in both models by adding a 7th value to fcondtopn_d. This makes its fcondtopn_f have identical values in both models. Adjust 'fsurfn_d' accordingly.
In 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26), the 'debug_model' namelist flag was added and calls to 'debug_ice' were added throughout the standalone model's 'ice_step'. This is useful for debugging model runs (in the scientific sense) since this subroutine prints the complete ice state, so calling it several times per time step can help diagnose where things go wrong. Do the same in the 'nemo_concepts' driver.
In our in-house CICE4 version, we had special initialization code in ice_flux::init_coupler_flux to initialize the 'fcondtopn_d' and 'fsurfn_d' arrays for ncat != 6. This was needed to avoid going out of bounds on the loop on ncat where these arrays are used below to initialize fcondtopn_f and fsurfn_f. This was fixed in CICE in 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26) by using the last (6th) value of the array for any higher category. This leads to 'fcondtopn_d' having different values in CICE6 for the last 4 categories compared to our in-house CICE4. To make side-by-side debugging of CICE6 and CICE4 easier, minimize the differences between the initialization in both models by adding a 7th value to fcondtopn_d. This makes its fcondtopn_f have identical values in both models. Adjust 'fsurfn_d' accordingly.
In 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26), the 'debug_model' namelist flag was added and calls to 'debug_ice' were added throughout the standalone model's 'ice_step'. This is useful for debugging model runs (in the scientific sense) since this subroutine prints the complete ice state, so calling it several times per time step can help diagnose where things go wrong. Do the same in the 'nemo_concepts' driver.
In our in-house CICE4 version, we had special initialization code in ice_flux::init_coupler_flux to initialize the 'fcondtopn_d' and 'fsurfn_d' arrays for ncat != 6. This was needed to avoid going out of bounds on the loop on ncat where these arrays are used below to initialize fcondtopn_f and fsurfn_f. This was fixed in CICE in 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26) by using the last (6th) value of the array for any higher category. This leads to 'fcondtopn_d' having different values in CICE6 for the last 4 categories compared to our in-house CICE4. To make side-by-side debugging of CICE6 and CICE4 easier, minimize the differences between the initialization in both models by adding a 7th value to fcondtopn_d. This makes its fcondtopn_f have identical values in both models. Adjust 'fsurfn_d' accordingly.
In 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26), the 'debug_model' namelist flag was added and calls to 'debug_ice' were added throughout the standalone model's 'ice_step'. This is useful for debugging model runs (in the scientific sense) since this subroutine prints the complete ice state, so calling it several times per time step can help diagnose where things go wrong. Do the same in the 'nemo_concepts' driver.
In our in-house CICE4 version, we had special initialization code in ice_flux::init_coupler_flux to initialize the 'fcondtopn_d' and 'fsurfn_d' arrays for ncat != 6. This was needed to avoid going out of bounds on the loop on ncat where these arrays are used below to initialize fcondtopn_f and fsurfn_f. This was fixed in CICE in 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26) by using the last (6th) value of the array for any higher category. This leads to 'fcondtopn_d' having different values in CICE6 for the last 4 categories compared to our in-house CICE4. To make side-by-side debugging of CICE6 and CICE4 easier, minimize the differences between the initialization in both models by adding a 7th value to fcondtopn_d. This makes its fcondtopn_f have identical values in both models. Adjust 'fsurfn_d' accordingly.
In 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26), the 'debug_model' namelist flag was added and calls to 'debug_ice' were added throughout the standalone model's 'ice_step'. This is useful for debugging model runs (in the scientific sense) since this subroutine prints the complete ice state, so calling it several times per time step can help diagnose where things go wrong. Do the same in the 'nemo_concepts' driver.
In our in-house CICE4 version, we had special initialization code in ice_flux::init_coupler_flux to initialize the 'fcondtopn_d' and 'fsurfn_d' arrays for ncat != 6. This was needed to avoid going out of bounds on the loop on ncat where these arrays are used below to initialize fcondtopn_f and fsurfn_f. This was fixed in CICE in 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26) by using the last (6th) value of the array for any higher category. This leads to 'fcondtopn_d' having different values in CICE6 for the last 4 categories compared to our in-house CICE4. To make side-by-side debugging of CICE6 and CICE4 easier, minimize the differences between the initialization in both models by adding a 7th value to fcondtopn_d. This makes its fcondtopn_f have identical values in both models. Adjust 'fsurfn_d' accordingly.
In 97370d7 (Update testing, in particular for diagnostics and decompositions (CICE-Consortium#602), 2021-05-26), the 'debug_model' namelist flag was added and calls to 'debug_ice' were added throughout the standalone model's 'ice_step'. This is useful for debugging model runs (in the scientific sense) since this subroutine prints the complete ice state, so calling it several times per time step can help diagnose where things go wrong. Do the same in the 'nemo_concepts' driver.
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
Update testing, in particular for diagnostics and decompositions
apcraig
Full test suite run on cheyenne with 3 compilers. Tests are bit-for-bit except boxslotcyl which had a change to the namelist settings (tr_lvl=tr_pond_lvl=.false.) and the calchk unit test which was set to 1000 years of testing by default, but should have been 100,000 years. Several new tests were added.
List of changes are below. Of particular note
These changes address many issues in #437 as well as lines 7-17 and maybe 81-82 in https://docs.google.com/spreadsheets/d/12tIfm5OzvzH_LF_ie9WbOn_w5aY1hv4SrqTQQ8bqM-s. Code coverage should go up after this update and that will be evaluated separately.
Otherwise, a summary of all changes follows