-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Distribute unablatedVolumeDynCell among neighboring cells in li_apply_front_ablation_velocity #27
Distribute unablatedVolumeDynCell among neighboring cells in li_apply_front_ablation_velocity #27
Conversation
I tested the The results on 2 and 5 nodes are not bit-for-bit, but this is using |
@trhille , recap of our discussion today about this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some minor suggestions. I don't think they will significantly affect the behavior.
do iEdge = 1, nEdgesOnCell(iCell) | ||
iNeighbor = cellsOnCell(iEdge, iCell) | ||
if ((li_mask_is_dynamic_ice(cellMask(iNeighbor))) .and. (bedTopography(iNeighbor) <= config_sea_level) & | ||
.and. (cellVolume(iNeighbor) > 0.0_RKIND) ) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to have a tolerance than using 0.0 here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me what that tolerance should be. Should it be something like 1 m thickness * areaCell(iNeighbor)
, some fraction of unablatedVolumeDynCell(iCell)
, or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining an eps
or tol
variable with a value of 1.0e-6 or something. The goal being just to catch round off level errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was addressed in commit 9202029.
components/mpas-albany-landice/src/mode_forward/mpas_li_calving.F
Outdated
Show resolved
Hide resolved
components/mpas-albany-landice/src/mode_forward/mpas_li_calving.F
Outdated
Show resolved
Hide resolved
components/mpas-albany-landice/src/mode_forward/mpas_li_calving.F
Outdated
Show resolved
Hide resolved
Still testing on commit b92225d before addressing @matthewhoffman's comments. I tested the MISMIP case with the imposed velocity field as before, using 36 and 72 procs. These are close, but not bit-for-bit, indicating that more work is needed. but there is an unsettling feature in the calving flux: Both the restart file output interval and |
I tested this using @matthewhoffman's calving test suite in MPAS-Dev/compass#318 and rebasing onto the branch in PR #28. The changes here give bit-for-bit results across different numbers of processors when the changes in PR #28 are included:
For reference, here are the test results using
Note that I had to change |
If a cell is left with unablated volume after step 2c in li_apply_front_ablation_velocity, distribute that volume among its dynamic neighbors. Testing with von Mises calving on the MISMIP+ domain indicates that this greatly reduces inaccuracies in the calving routine, but that very large calving events can still trigger the >10% calving inaccuracy error. This could potentially be alleviated by adding a do-while loop over the new section 3 in li_apply_front_ablation_velocity, but we may want to control for extremely large calving events anyway.
…on in step 3 Require unablatedVolumeDynCell(iCell))>0.0_RKIND when applying ablation in step 3 in li_apply_front_ablation_velocity to avoid doing extra calculations where they do not apply.
d8d4de3
to
a4cfdad
Compare
Use a tolerance consistent with round-off level ice thickness in li_apply_front_ablation_velocity instead of requiring cellVolume > 0.0.
4bb2fda
to
9202029
Compare
Add a namelist option to turn distribution of unablatedVolumeDynCell to neighboring dynamic cells on or off. Default is to have this feature enabled, but it may be desirable to have it disabled to ensure that results are not strongly dependent on this ad hoc cleanup, and that the primary control on the accuracy of calving results comes from the length of the timestep (i.e., that the code is not violating a CFL-like condition based on ablation velocity).
I rested full_integration suite against current baseline, and all tests pass:
I also tested the humboldt_calving_tests suite (without a baseline), and all tests pass except the one that was already failing:
It is likely that none of these tests exercise the new code, because if they did, they probably would have been generating runtime errors before this. That said, I confirm this code does no harm. @trhille's initial testing above on custom cases where the calving inaccuracy occurs look solid. More careful testing will be done in conjunction with testing for #33 . Any issues that come up can be revisited then (including if we want this on by default). |
If a cell is left with unablated volume after step 2c in li_apply_front_ablation_velocity, distribute that volume among its dynamic neighbors to prevent large errors in the ablated volume. This can be turned on or off using the
config_distribute_unablatedVolumeDynCell
namelist option. Testing with von Mises calving on the MISMIP+ domain indicates that this greatly reduces inaccuracies in the calving routine, but that very large calving events can still trigger the >10% calving inaccuracy error. However, this should only be a secondary means of getting accurate calving results; the primary means should be by using an appropriately small timestep determined by the ablation velocity in a CFL-like condition.