FSD fixes for conservation #495
Conversation
dabail10
requested review from
eclare108213,
apcraig,
TillRasmussen and
phil-blain
columnphysics/icepack_therm_itd.F90
Outdated
| ! floe size distribution | ||
| if (tr_fsd) then | ||
| if (rsiden(n) > puny) then | ||
| if (aicen(n) > puny) then ! not sure if this should be aicen or aicen_init |
There was a problem hiding this comment.
What's the plan for figuring out if it should be aicen or aicen_init? Why not use the most recent value? I guess the vertical melting and lateral melting are proceeding simultaneously, so neither is exactly correct. (aicen+aicen_init)/2?
There was a problem hiding this comment.
Actually, is aicen_init at the beginning of the timestep or some other point during the timestep? Seems to be set at the beginning of the lateral_melt subroutine.
There was a problem hiding this comment.
I would think this should be aicen_init, which isn't defined yet. Otherwise aicen might be less than puny from the state variable adjustment immediately above, and the fsd wouldn't be updated equivalently.
There was a problem hiding this comment.
When I look at this code, it seems like the if conditions on rsiden and aicen are kind of redundant. Except in the case where rsiden(n) = 1. This would mean that aicen(n) would go to zero. So, I do think you want a check on the updated aicen(n).
There was a problem hiding this comment.
If you have concluded that aicen(n) is correct, then please remove the "not sure" comment!
columnphysics/icepack_therm_itd.F90
Outdated
| @@ -1251,37 +1172,37 @@ subroutine lateral_melt (dt, ncat, & | |||
| if (z_tracers) & | |||
| call lateral_melt_bgc(dt, & | |||
| ncat, nblyr, & | |||
| rside, vicen_init, & !echmod: use rsiden | |||
| rsiden(1), vicen_init, & !echmod: use rsiden !CMB this should be sent rsiden | |||
|
GHActions/IcepackTesting failed, so I relaunched it. |
|
One of the failed tests is a Picard convergence. I will do some offline testing. |
|
This seems to change answers for all configurations. Is that expected? There is also an "out of memory" error for one of the tests. The derecho test suite, https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#925bdea8760a8d833f0550b408226e80a8969378, seems to have the same errors thrown up by github actions. Please add more information to the PR section "Please document the changes in detail, including why the changes are made. This will become part of the PR commit log" listing and explaining what things changed in greater detail. Also, you indicate a dependency on CICE, does this mean public Icepack interfaces changed and if so, please document that. Good to also explain why answers changed in a little greater detail. Thanks. |
|
This test is failing in github actions. This same test is fine on derecho_intel. testsuite.macos-latest/conda_macos_restart_col_1x1_swccsm3.macos-latest/logs/icepack.runlog.240730-182013(picard_solver) picard_solver: Picard solver non-convergence (icedrv_system_abort) ABORTED: |
|
You could try running that test on derecho with other compilers (gnu, cray?) and also turn on the debug flags to see if it picks anything up. Could this be a fluke that is related to #494? Have you tried running in CICE and does the test suite run fine there? Are there some modifications that could be impacting this test in particular? |
|
I am able to reproduce it on my macos conda environment. There are NaN's in the mushy-layer solution. This case is using the old shortwave (ccsm3) with ktherm=2. I don't think we should be testing this. |
Can you suggest changes to the swccsm3 test? Always good to review our test suite and continue to make improvements. Feel free to also suggest a couple new tests. |
|
I am testing this out with ktherm=1. I think this might be better. |
|
What else is needed here? Can I change the swccsm3 to use ktherm=1? |
Yes. I'll look at my previous comments to see what can be marked as resolved. |
|
Ok. I have cleaned up the driver code and only the changes related to rside / rsiden are now there. |
|
Weird. It is emailing me and telling me gh-actions is failing. |
|
The GH Actions failures are happening with some regularity these days. Restarting them seems to work. Not clear what's going on. |
…s a memory issue.
|
I thought I had addressed all of @eclare108213 's comments above. I guess I need some help prototyping these arrays. |
I resolved the ones that you addressed, and there are still a few left. Thanks. |
|
No problem, let me create a sandbox with the mods and try to propose something. It could be I'm misreading the code too. |
c_fsd_range in icepack_init_fsd_bounds to pass the arrays back to the driver. Pass afsdn into icepack_stgep_therm1 as an optional argument, check it exists with tr_fsd, and pass it down the calling tree as optional. Remove fsd variables that are not used in the Icepack driver. Clean up some indentation and blank spaces Update Icepack interface documentation
Update fsd
I am not finding the unresolved comments. I found one about fside / rside as diagnostics. I think I will add these later. |
Scroll through “Files changed”. |
|
Hopefully this is ready for review again. |
columnphysics/icepack_therm_itd.F90
Outdated
| ! floe size distribution | ||
| if (tr_fsd) then | ||
| if (rsiden(n) > puny) then | ||
| if (aicen(n) > puny) then ! not sure if this should be aicen or aicen_init |
There was a problem hiding this comment.
If you have concluded that aicen(n) is correct, then please remove the "not sure" comment!
|
This PR also redefines |
|
@dabail10, can you review the latest comments and update the code if needed. I will start testing again too. Can we get this merged in the next couple days? Once we merge this, we'll have to update the CICE fsd PR to point to the merged Icepack. Trying to hold other PRs at the moment, hoping we can finish this up. |
|
I will try to resolve the remaing issues tomorrow. |
|
I have resolved the most recent issues. |
|
I'm running a test suite on derecho. Unless I hear otherwise, I'll assume the fsd results are as expected. Will approve and merge when testing is complete, again, unless others provide additional review/feedback. Are any updates needed in the PR summary? I will cut and paste that into the merge comment. |
I did update the PR summary as requested by @eclare108213. |
|
I just updated the PR summary as well. Are any updates needed in the user guide for the fsd changes? |
Not at this point. This was just a redo of the code. The options for activating it have not changed. I will have a future PR that will be bringing in an alternative wave fracturing method and the documentation will change then. |
|
Testing looks good. |
|
All reviews have been resolved. Will merge now. @dabail10, can you update Icepack in the CICE PR to point to the consortium main when you have a chance, then we can merge that too. Thanks. |
This is a bug fix plus some rearranging for conservation in CICE. This impacts FSD cases as well as non-FSD cases.
There are two main aspects to the bug fix:
Initial ice and snow volume values at the beginning of the lateral melt routine are now used for updating the snow and ice enthalpy as well other tracers for lateral melting. This is answer changing in all cases!
The other fix is to move the computation of vi0new_lat (lateral growth from FSD) outside of the subroutine fsd_lateral_growth and a check to make sure the category vi0new (vin0new) does not exceed the total vi0new. This is a warning and it recomputes to ensure conservation. This is only answer changing in FSD cases.
There is also a change in definition of floe_area_c, so that it is precisely half way between floe_area_l and floe_area_h. The original computation of floe_area_c based on floe_rad_c biases the area towards the lower value.
Update the fsd subroutine arguments (floe_rad_c, floe_rad_l, floe_binwidth, c_fsd_range) to store static data inside Icepack when computed in Icepack rather than pass them back and forth. Provide an ability to pass out the values from Icepack as optional arguments.
Move the computation of rsiden and get rid of fside which is no longer needed.
Clean up some comments and indentation
Update swccsm3 test, set ktherm=1
Update interface documentation
There is an associated CICE tag to go with this.
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
This fixes the conservation issues in CICE with the FSD, but also for non-FSD cases!
@dabail10 , @lettie-roach , @cmbitz
https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#925bdea8760a8d833f0550b408226e80a8969378
This is a bug fix plus some rearranging for conservation in CICE. This impacts FSD cases as well as non-FSD cases.
There are two main aspects to the bug fix:
Update the fsd subroutine arguments (floe_rad_c, floe_rad_l, floe_binwidth, c_fsd_range) to store static data inside Icepack when computed in Icepack rather than pass them back and forth. Provide an ability to pass out the values from Icepack as optional arguments.
Move the computation of rsiden and get rid of fside which is no longer needed.
Clean up some comments and indentation
Update swccsm3 test, set ktherm=1
Update interface documentation
There is an associated CICE tag to go with this.