Implement Floe Size Distribution (FSD) in Icepack #281
Conversation
First draft adding FSTD definition
Add FSD lateral melt
Adding initial draft of FSD documentation
Added note on lateral growth
Adding floe size distribution references
|
Reminder to self: there is at least one subroutine name in the fsd code that starts icepack_, which is not an interface routine. Fix that. |
Personally, I think all the subroutines in icepack should start with icepack_ and be named such that you could even tell which module/file they're in. icepack_step_radiation -> icepack_shortwave_step_radiation and run_dEdd -> icepack_shortwave_run_dEdd. I think that's not necessary or practical at this point, but I don't have a problem with non-public subroutines starting with icepack_. |
There was a problem hiding this comment.
This looks fine, lots here. I thought some of the new fsd arguments were going to be implemented fully "optional" in the public icepack interfaces. If fsd is not used, the arguments still have to be passed? I think we'd like to move away from that style of implementation and was hoping fsd might be the first feature that was implemented that way. I wouldn't change it now though.
I am running some test suites and will report those results soon. Once done, I'll approve.
|
Thanks for the reminder - it would be good to change the fsd arguments to be optional if we are going to impose that on new code going forward. I guess I was thinking that would be part of the icepack interface upgrade. Looking through the code, though, this would create a LOT of "if(present())" checks, which doesn't seem like a good idea. Would the optional variables only be in the icepack interface, and if they are present then load their values into non-optional arguments passed into lower-level subroutines (which would be ignored if tr_fsd=F)? Or assume that if tr_fsd is true, then the optional arguments are indeed passed? I just noticed a bug in line 1069 of ice_therm_itd.F90: it should be rhoivicen+rhosvsnon |
|
The optional argument implementation questions are big ones. We certainly would like to move toward optional arguments in the public interfaces. The way this is most easily handled is to move toward module data within Icepack and move away from arguments where possible. But that creates other tradeoffs. Sometime before, I noted a few ways to handle the optional arguments. 1) move to module data where the flags we have trigger different features and move away from arguments inside Icepack. 2) lots of "if present" followed by lots of calls to the same interfaces with different arguments. 3) present checks in the public interfaces that instantiate dummy arguments to pass further down the calling tree as needed. None are ideal. There are other options too like refactor the code so different features are implemented via their own interfaces. We are partly in this situation because the Icepack library came out of a working code instead of being designed as a library itself from the start. The more I look at and understand the Icepack code, the less sure I am that we can overcome that without a lot of work. I think it's fine to leave the fsd arguments as is for now and look at making them optional as part of the current interface refactor effort. Is the ice_therm_itd bug an answer changer? If so, I recommend we make that a separate and clean PR, so the fsd and interface refactors both can be tested via bit-for-bit comparisons more easily. |
|
Testing looks good. Full test suites with 4 compilers on each of gordon and izumi pass. See hash 9b4af6b at https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks. One test failed on izumi intel which I believe is not something to be concerned about. The comparison fails are for fsd tests where there are no baselines. |
|
Please fix the bug first, noted in my previous comment.
|
|
OK, I've fixed the bug in icepack_therm_itd in my sandbox and am retesting icepack. If this is an answer changer, I still think we should consider fixing it as a separate PR though to preserve bit-for-bit in the fsd PR. |
|
Okay, let’s continue to discuss the interface implementation and go ahead with FSD. The bug was introduced with my FSD changes and so should be fixed in this PR. I think it only affects a coupling field so wouldn’t show up as answer changing in stand-alone runs.
|
|
Sounds good @eclare108213 thanks. |
|
Also, please check the to-do list at the top of this PR. I think it’s all done but want to confirm. Seems like there was still a box left unchecked.
|
|
The box left unchecked was testing with -s fsd1 vs current master for bit-for-bit. I added a comment that all tests pass with -s fsd1 but it is NOT bit-for-bit with master. Do we need to look into this? |
|
No, I wouldn’t expect it to be BFB. Miracle if it were! I wonder if it’s worth running through QC as a sanity check, though.
|
|
Is it not bit-for-bit because of order of operations or because a feature is actually turned on that changes answers? I could do a qc test, will put that on my list. |
|
Essentially order of operations. This test uses the FSD code instead of the hard-wired floe size in the original code, but is supposedly configured equivalently (a single, constant, 300 m floe size). So QC should pass unless there is something wrong with the FSD code.
|
|
Testing with the icepack_therm_itd fix went fine, everything passed and was bit-for-bit. I have pushed that fix to the branch. |
|
I ran a qc test with the current fsd cice branch (3b487bde1bd12) and the current icepack branch (cf1ecc0) which includes the fix to icepack_therm_itd. I ran two qc tests, one with out-of-the-box settings and a second, identical case with tr_fsd=.true. set manually in ice_in. The qc test seems to have failed,
|
|
That is disappointing. I'll look into it. First, could you check some things in the diagnostic output for the fsd run? It should have tr_fsd=T, nfsd=1, wave_spec_type=none in namelist; it looks like defaults are set appropriately so these should be okay. The other thing to check is the floe size, 'avg fsd rep radius' in the log file, to see whether it is approximately 300 m. I don't know whether to expect it to stay exactly 300. I'll work on this, hopefully Monday, but @apcraig if you could check the output, that would give me a head start. Thanks for running the tests! |
|
I had a quick look at an nfsd=1 run I'd done earlier, and it's showing the avg radius ~150 m. This means the floe size bin isn't initialized right. In icepack_fsd.F90, try changing |
|
I take it back, the floe size diameter should be 300 m, so the radius should be 150 m. The small difference due to the lower bound might make be making a difference for the QC -- I'm not sure how sensitive it is. This could be tested: in icepack_fsd.F90, try changing |
|
I ran two additional qc tests with
Running the t-test, all the cases with tr_fsd=.true. and different lims settings pass the qc test against each other (and are not bit-for-bit). But none of the tr_fsd=.true. cases pass versus the case with tr_fsd=.false. It seems like there is a difference in the two implementations (tr_fsd = false and true). What part of the code should we be looking at? Also, in the long term, we probably want just one implementation that handles either tr_fsd setting to reduce maintenance among other things. If we have two, as either is upgraded, the other has to follow. |
|
@apcraig, thanks for running these tests. I'll compare history output from CICE runs with and without tr_fsd on, tomorrow, to see whether/how they significantly differ, and maybe debug in Icepack. I agree that we should have only one implementation for nfsd=1, and that should be the original (tr_fsd=F), since it's a lot cheaper to run. But this test is a useful sanity check for the FSD code. I'm glad we set it up! |
|
I have looked into the nfsd=1 case and now realize that the FSD operates very differently from the standard code, in that each thickness category is associated with a whole distribution of floe sizes. This means that, e.g. when new frazil ice is added, it is distributed among all of the different ice thickness categories, while in the standard code, we add all of the new ice to the thinnest thickness category unless there's not room for the whole volume, in which case it's spread among categories. So the calculations for tr_fsd=T / nfsd=1 are fundamentally different from tr_fsd=F. It's reassuring that nfsd=1 maintains the floe diameter that it starts with, and I think we should consider this good enough for this PR. I believe this PR is ready to be merged, pending final changes to and tests of the full CICE FSD PR. |
Implement Floe Size Distribution (FSD) in Icepack (CICE-Consortium#281)
* add tdir cice.setup option to define test directory * update documentation
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
Implement floe size distribution in Icepack
@lettie-roach and @eclare108213
118 of 130 tests PASSED
9 of 130 tests FAILED
0 of 130 tests PENDING
https://github.com/CICE-Consortium/Test-Results/wiki/9b5a5670e4.badger.intel.19-10-31.041919
This includes FSD tests not available in the original code, so there are some 'misses'. The failures are all expected -- the code is set to abort if the FSD and BGC are both turned on, until results from the combination can be looked at and fixed as needed.
The current Icepack tests do not exercise wave breaking, which can be tested in CICE; a simpler form might be included in Icepack in the (near?) future. Results from the current tests need closer scrutiny to make sure they are behaving as expected.
The default value of ice_ic is changed in icepack_in, to make the functionality the same as in CICE for the FSD. This does not matter for general icepack initialization because each of the 4 grid cells is initialized manually, and ice_ic is only meaningful for restart runs.
There are a few more things to do, before the FSD is ready for use in CICE. Most of what is needed for Icepack is here, but needing review and additional testing. In particular:
See also the FSD project board.