Skip to content
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

ridge_ice no longer public #478

Closed
kshedstrom opened this issue Nov 21, 2023 · 19 comments · Fixed by #503
Closed

ridge_ice no longer public #478

kshedstrom opened this issue Nov 21, 2023 · 19 comments · Fixed by #503
Assignees

Comments

@kshedstrom
Copy link

Hi all, some of us in the SIS2 community have been calling Icepack's ridge_ice from SIS2. I saw that it became private as of Icepack 1.3.4 and started working on updating how we call Icepack's ridging routines. The public icepack_step_ridge calls both ridge_ice and cleanup_itd. I'm not opposed to the idea of cleaning up the itd in a mass and energy conserving fashion, but something you are doing doesn't play well with SIS2.

  • It flat out doesn't work when we use a process mask_table
  • Without a mask_table it makes the coupled ice-ocean model unbelievably slow, slower than the simple lack of the mask_table can explain.

Can I ask that ridge_ice be made public again?

@apcraig
Copy link
Contributor

apcraig commented Nov 22, 2023

We could make it public. We would rename it to something like icepack_ridge_ice if we do. We'll have to discuss that. But I'd like to understand better why icepack_step_ridge doesn't work. When you say "process mask_table", what do you mean.

Also, are you currently just using ridge_ice more-or-less without any other icepack code? If we make it public, maybe that would be a good time to update the public ridging interfaces to remove all the config/parameters arguments that are static during the run. These are set with the icepack_init_parameters interface prior to using ridge_ice. Would that be problematic?

@kshedstrom
Copy link
Author

Both MOM6 and SIS2 have the option to read a mask_table such that the cores with all land can be masked out. For instance, I can have a layout of 20 by 25, with say 40 of them masked out. One would then request 460 cores to run it on instead of 500. When running SIS2 with icepack_step_ridge and a mask_table, it fails in a communication elsewhere in the code.

The SIS2 ridging code has an initialization routine which calls icepack_init_tracer_sizes, icepack_init_tracer_indices, and icepack_init_parameters(mu_rdg_in=CS%mu_rdg, conserv_check_in=.true.). I don't have a problem with more such calls.

I believe the plan is to make more calls to Icepack in the future, adding features currently lacking in SIS2, but not CICE. Melt ponds have been discussed, also the light penetration scheme and black carbon. Maybe eventually even the ice algae.

@apcraig
Copy link
Contributor

apcraig commented Nov 22, 2023

Thanks @kshedstrom. The icepack calls are on a gridcell by gridcell basis, so however you mask and decompose has nothing to do with icepack. There is no dependency between gridcells in icepack, so it'd be interesting to understand better why changes to icepack are interacting with the mask/decomposition.

@kshedstrom
Copy link
Author

According to icepack_itd.F90:

! Cleanup subroutine that rebins thickness categories if necessary,
!  eliminates very small ice areas while conserving mass and energy,
!  aggregates state variables, and does a boundary call.

I didn't immediately find the "boundary call", assuming that is some sort of MPI communication. I wouldn't know what to look for!

@apcraig
Copy link
Contributor

apcraig commented Nov 22, 2023

@kshedstrom that comment must be old. There is no boundary call in Icepack. Icepack operates on one gridcell at a time with no communication between gridcells in icepack.

@kshedstrom
Copy link
Author

@apcraig Interesting. I can't explain my troubles with it then.

@eclare108213
Copy link
Contributor

We need to fix the boundary call comment in the code, but otherwise it would be good to understand why icepack_step_ridge is causing a problem. It sounds like cleanup_itd is the source, since ridge_ice works on its own, but perhaps there is something else in icepack_step_ridge, like an array declaration or intent() or optional argument? What communication elsewhere fails? Do you get any additional information when running with debugging flags in a case that fails to see if something isn't initialized, NaNs etc? The significant slowdown could be indicative of NaNs.

@eclare108213
Copy link
Contributor

Hi @kshedstrom, did you ever figure out what was causing the boundary problem when calling icepack_step_ridge? Based on the discussion above, it shouldn't be coming from Icepack itself, and if it's no longer a problem for you, I'd like to close this issue.

@kshedstrom
Copy link
Author

No, sorry, it's just easier for us to checkout version 1.3.3 and use that. If I tell Icepack not to call cleanup_itd, I get a lot of warnings from Icepack and some options cause SIS2 to blow up as well in its dynamics. I need a smaller test case!

@eclare108213
Copy link
Contributor

Does it work to use the current release but just make ridge_ice public? That's a simple fix on your end. There definitely seems to be a conflict of some kind between SIS2's setup and Icepack, whether it's memory or undefined array arguments or something else.

@kshedstrom
Copy link
Author

No, it doesn't work. Something has changed numerically between 1.3.3 and 1.4.x.

One option if you don't allow the public ridge_ice is to add some optional arguments as in https://github.com/ESMG/Icepack/tree/optional_cleanup
but that doesn't work for me either.

Sorry, recovering from Covid, moving a little slowly.

@kshedstrom
Copy link
Author

If I make it public, the answers change in commit acfc046

from:

ice_ridge before   1.0051568350814692       0.84627697364914511
ice_ridge after   1.0000000000000000       0.84627697364914523

to:

ice_ridge before   1.0051568350814692       0.84627697364914511
ice_ridge after  0.99940937552052012       0.84223837117108713

These numbers are the sum of aicen and vicen at one point. I also now get all these
warnings:

  (column_conservation_check)Conservation error: (ridge_ice):vice
  (column_conservation_check)  Initial value =    1.1733997376745637
  (column_conservation_check)  Final value   =    1.1733861081236896
  (column_conservation_check)  Difference    =   -1.3629550874139085E-005

@kshedstrom
Copy link
Author

OK, I figured it out. It is in how ridge_ice is called. All will be well if ridge_ice can be made public.

@kshedstrom
Copy link
Author

Should I submit a PR making ridge_ice public or should I submit the other way, having new optional arguments to icepack_step_ridge?

@apcraig
Copy link
Contributor

apcraig commented Oct 1, 2024

I can help too. The optional arguments look pretty reasonable. It looks like there are just two optional flags that trigger some relatively simple stuff. This seems preferable to making ridge_ice public. @eclare108213 ? @kshedstrom do you have a particular preference for making ridge_ice public?

@kshedstrom
Copy link
Author

I don't care which way we go, but my PR to SIS2 depends on having one or the other in place.

@eclare108213
Copy link
Contributor

The optional arguments seem like a reasonable solution in this case, so let's go with that.

I still wonder why there's a problem to begin with. The rebin subroutine that you are avoiding has this comment:

  ! Make sure ice in each category is within its thickness bounds.
  ! NOTE: The rebin subroutine is needed only in the rare cases
  !       when the linear_itd subroutine cannot transfer ice
  !       correctly (e.g., very fast ice growth).

By not calling rebin and cleanup, you are exiting the mechanical redistribution process with an ice state that isn't properly binned in the ice thickness categories. What happens in that case? Does the horizontal transport then fix the binning for you?

Answering that question still doesn't explain the original failure when you do call those routines. Icepack columnphysics has no grid dependency, and so MPI or other grid-related communication failures are coming from SIS's configuration, somehow. Do you get NaN's when you run the coupled ice-ocean model without a mask_table? That's my guess for the very slow runtime in that case.

@apcraig apcraig self-assigned this Oct 2, 2024
apcraig added a commit to apcraig/Icepack that referenced this issue Oct 2, 2024
… support

NOT calling cleanup_itd and NOT calling rebin in the ridge_ice.

Closes CICE-Consortium#478
@apcraig
Copy link
Contributor

apcraig commented Oct 2, 2024

I have created a draft PR based on @kshedstrom branch. See #503

Looking for feedback, review, and testing as needed.

@kshedstrom
Copy link
Author

I'm afraid the main computer I use (chinook) is being impossibly slow this week. I will have to try some things on another computer. I've looked into it some and the real problem I had in the transition from 1.3.3 to 1.4 is that we took some shortcuts and set a lot of the arguments to zero, counting on ridge_prep to do some of the computations. When calling the new ridge_ice, I could omit the optional arguments opening and closing to get the old behavior. I will try doing the equivalent computation on the SIS2 side and get back to you.

apcraig added a commit to apcraig/Icepack that referenced this issue Oct 23, 2024
… support

NOT calling cleanup_itd and NOT calling rebin in the ridge_ice.

Closes CICE-Consortium#478
apcraig added a commit that referenced this issue Oct 24, 2024
)

Add dorebin and docleanup optional arguments to icepack_step_ridge to support NOT calling cleanup_itd and NOT calling rebin in the ridge_ice.
Closes #478

Rename limit_aice_in argument in ridge_ice to limit_aice. This argument is not used in Icepack or CICE at the moment.

Update interface document

Based on https://github.com/ESMG/Icepack/tree/optional_cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants