update icepack to support snowtable better #1
Conversation
| ! if snw_aging_table = 'file' | ||
| ! all data has to be passed into icepack_parameters | ||
|
|
||
| ! tcraig, should be 223.15 and 243.15 below |
There was a problem hiding this comment.
Ah, this is what's different with when reading the file. I think these should be fixed here, now.
If we do that, then is there any reason to have the 'snicar' option? Speed?
There was a problem hiding this comment.
Actually, the documentation isn't quite correct. The 11 temperatures goe from 223 to 273 by 5 while on the actual snicar file they go from 223.15 to 273.15 by 5. That is the "bug" in the hardwired "snicar" values.
The only reason to keep snicar is if there are table input file that do not have the 1D data on them. Or if we want to preserve the old 1D values which may not be correct.
There was a problem hiding this comment.
@njeffery
@apcraig turned up an inconsistency between the snow aging file that I gave him (which was the most recent one on LANL IC machines, snicar_drdt_bst_fit_60_c04262019.nc), where the code uses 223.0 as the starting T index but the file has 223.15. My inclination is to fix this in the code, but I want to make sure that the file I'm using is correct.
Alternatively, we can dispense with the index calculation in the code completely, and just read it from the file. That's much better from a software perspective. Thoughts?
There was a problem hiding this comment.
I double checked the snicar file and it's correct. Also, the ELM code uses an integer 223 in this expression, not 223.15.
| endif | ||
|
|
||
| ! boundary check: | ||
| rhos_idx = min(isnw_rhos, max(1,rhos_idx)) |
There was a problem hiding this comment.
These indices are different by +1. Is this a bug in the original code?
There was a problem hiding this comment.
I moved the +1 into the index calculation. If you look at the previous implementation, it was part of the index check in the next code block. I thought it was clearer to put the +1 in the computation of the index then check the index cleanly vs min/max after.
|
Should probably also update icepack_in and set_nml.snw* to be consistent with CICE's. Will make a note to do that... |
PR checklist
Update Icepack to support the snowtable better
apcraig
Tested with CICE changes, full test suite on cheyenne with intel has all tests pass but are not bit-for-bit with the current Consortium master (expected). https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#8a90b84895c3a42806db0345a4183e22715ee066. This code should be bit-for-bit with the base though.