Bring in snowmip diagnostic fields #449
Conversation
https://svn-ccsm-models.cgd.ucar.edu/clm2/branch_tags/SSREsnowmip_tags/SSREsnowmip_n00_clm4_5_18_r272 NOTE: Even though this says it's a "n00" version it does actually contain changes from the baseline version clm4_5_18_r272. It really should be labeled "n01".
https://svn-ccsm-models.cgd.ucar.edu/clm2/branch_tags/SSREsnowmip_tags/SSREsnowmip_n00_clm4_5_18_r272 NOTE: Even though this says it's a "n00" version it does actually contain changes from the baseline version clm4_5_18_r272. It really should be labeled "n01".
Add some new diagnostic fields. Some needed for CMIP6. Update the CMIP6 user-mods output. Fix a couple issues. Get full list of history tapes working correctly. Check for valid range of CO2. New IC file interpolated from the previous one for f19_g17_gl4 for 2000 Clm50BgcCrop.
ekluzek
added
priority: high
|
OK, looking at this I realize more work needs to be done on this. There's some name comments that need to come out. It also currently doesn't test the new fields, so I've done some testing with use_ssre turned on and that's looking fine. So I also need to add a test with the new fields. And it looks like there maybe some duplication that could be eliminated that I'm going to look into. |
|
The following tests all work fine with use_ssre=T. So that gives me some confidence that the new option is working. SMS_Lm1_D.f10_f10_musgs.I2000Clm50BgcCrop.cheyenne_intel.clm-snowlayers_3_monthly |
|
I can see that the duplication has already caused problems, because bug 2431 was fixed in the old, but not new version of TwoStream in SurfaceAlbedoMod. |
area is currently wrong on it. Also is missing time_bnds coordinate that is mentioned in the attribute on the time coordinate.
… some extra printing, make sure NCL thinks the vertices are clockwise, tried this version out in CLM runs at single point and f19 and it seems to work fine
in NetCDF format.
src/utils/clm_time_manager.F90
Outdated
| lon = londeg | ||
| if ( lon < 0.0_r8 ) lon = lon + 360.0_r8 | ||
| get_local_irrig_time = modulo(secs + nint(londeg/degpsec), isecspday) | ||
| get_local_irrig_time = modulo(get_local_irrig_time,isecspday) |
There was a problem hiding this comment.
Do you need this second modulo? There's already a modulo in the line above.
| londeg = 360.0_r8 | ||
| @assertEqual( expected, get_local_time( londeg ) ) | ||
|
|
||
| ! Check for local noon on otherside of the world |
There was a problem hiding this comment.
For this and some of the other tests you added: I find it significantly easier to understand what tests are doing if they follow the generally-recommended unit testing organization of arrange-act-assert (or equivalently: setup-exercise-verify). That is, each test should test one thing. It gets hard to follow tests when they call the system under test multiple times in a single test.
There was a problem hiding this comment.
@billsacks and I talked about this. The general advice is to have one test per thing tested to make it easiest to read. It also helps in the theoretical sense so that broken tests will all be shown rather than just the first assert that fails within a test. Bill didn't think this had to change for this PR, but we do want to try to follow best practices for unit testing in general.
src/utils/clm_time_manager.F90
Outdated
| call get_curr_date(yr, mon, day, secs, offset ) | ||
| lon = londeg | ||
| if ( lon < 0.0_r8 ) lon = lon + 360.0_r8 | ||
| get_local_time = secs + nint((lon/degpsec)/real(dtime,r8))*dtime |
There was a problem hiding this comment.
The division by dtime here yields incorrect results. See #491 (comment) for details.
There was a problem hiding this comment.
@billsacks and I went over this with @olyson and we figured out why this was done this way. This is just to make sure the code in SurfaceRadiationMod.F90 would match the closest time-step to local noon. This can be handled appropriately with the is_near_local_noon function. So we can remove the dtime logic that's here. The bottom line is that we want to use the method used in Irrigation for going forward. I still plan to have a bit-for-bit tag, that has two different methods and then we'll remove one with a tag that has answer changes.
|
@billsacks thanks for the review! I'll go back through and make changes and/or comments as appropriate. Note, that I went through several iterations of the compare_local_time unit test and part of how I knew it was working, was changing the delta degree. The last thing I did was to change the time it ran over from just a day, to a full year. I never got it to fail running with gnu on my laptop, but I didn't run it anywhere else. I'll comment more later. |
This shows the same answers for at least one of the cases that was different.
…igation version the standard name
… it to work on hobart with the nag compiler
…all off, as well as turning off the SSRE snowmip fields
ekluzek
added
PR status: ready
Bring in snowmip diagnostic fields
Description of changes
Adds in new fields needed for SNOWMIP
Specific notes
Contributors other than yourself, if any: flanner, swensosc, justinperket
CTSM Issues Fixed (include github issue #):
Are answers expected to change (and if so in what way)? No (only new diagnostic fields added)
Any User Interface Changes (namelist or namelist defaults changes)?
Add use_SSRE namelist item
Testing performed, if any:
Ran aux_clm test on cheyenne and hobart and tests work as expected and are bit-for-bit
(however, new fields weren't on by default)