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

Add subglacial runoff #82

Conversation

irenavankova
Copy link

This adds subglacial runoff output from a file (e.g. MALI output) at the grounding line as freshwater volume flux at local freezing point. It follows closely what river runoff does.

@xylar xylar changed the base branch from master to alternate March 29, 2024 14:13
@xylar xylar changed the base branch from alternate to master March 29, 2024 14:13
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@irenavankova, this looks like excellent work, as I always expect from you.

There are some small formatting fixes in the Registry to do.

The other issue, which we should discuss, is whether we want a namelist option and a package to prevent allocating subglacial runoff variables when they aren't used.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick formatting suggestion. I think this is otherwise ready to move to E3SM

components/mpas-ocean/src/Registry.xml Outdated Show resolved Hide resolved
@irenavankova
Copy link
Author

Not yet ready to move, I am on only half way putting in the packages - I introduced them but now need to add iffstatements all over the code, checking for package when using the subglacial variables

@xylar
Copy link
Collaborator

xylar commented Apr 3, 2024

Okay, no worries. Ping me when the time is right to have another look.

@irenavankova irenavankova force-pushed the ocn/add-subglacialRunoff branch 2 times, most recently from 2be9882 to fbe87a7 Compare April 29, 2024 19:45
@irenavankova
Copy link
Author

@xylar, I made the subglacial stuff into a package now - I think it is ready for you to have a look again when you have time.

@xylar xylar changed the base branch from master to alternate May 30, 2024 13:24
@xylar xylar changed the base branch from alternate to master May 30, 2024 13:24
Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is amazing! And nearly there. I just have a few small pieces of clean-up I'd recommend. Then, this will need to be rebased because of the TMIX PR that just went in (perhaps among other conflicts). And then it's ready to go to E3SM!

Happy to help with the rebase if you need.

@@ -305,6 +312,7 @@ add_default($nl, 'config_land_ice_flux_ISOMIP_gammaT');
add_default($nl, 'config_land_ice_flux_rms_tidal_velocity');
add_default($nl, 'config_land_ice_flux_jenkins_heat_transfer_coefficient');
add_default($nl, 'config_land_ice_flux_jenkins_salt_transfer_coefficient');
add_default($nl, 'config_subglacial_runoff_mode');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option got moved to the forcing namelist group in the registry so it should be moved up here as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 1672 to 1703
<entry id="config_subglacial_runoff_mode" type="char*1024"
category="land_ice_fluxes" group="land_ice_fluxes">
Selects the mode in which subglacial runoff fluxes are computed.

Valid values: 'off', 'data'
Default: Defined in namelist_defaults.xml
</entry>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be moved up to the forcing group.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<var name="fractionAbsorbedSubglacialRunoff" type="real" dimensions="nVertLevels nCells Time" units="fractional"
description="Divergence of transmission through interfaces of surface fluxes below the surface layer at cell centers. These are applied only to subglacial runoff."
packages="dataSubglacialRunoffFluxPKG"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/>
/>

Maybe add back the blank line that was here before, just for visual separation between fields for different purposes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -180,6 +182,48 @@ subroutine ocn_forcing_build_fraction_absorbed_array(meshPool, statePool, forcin
end do
end do

! now do subglacial runoff separately
if ( trim(config_subglacial_runoff_mode) == 'data' ) then
if ( trim(config_sgr_flux_vertical_location) == 'top' ) then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to add and else this this and raise an error if config_sgr_flux_vertical_location has an unexpected value? Otherwise, I worry that a typo could run fine but have weird and hard-to-explain behavior.

Copy link
Author

@irenavankova irenavankova Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is an appropriate way to do this? How to raise error and kill the run?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -3411,6 +3439,37 @@ subroutine ocn_compute_KPP_input_fields(statePool, forcingPool, &
- exp( max(-100.0_RKIND, &
-layerThickness(kmin, iCell)/ &
config_flux_attenuation_coefficient_runoff))
if (trim(config_subglacial_runoff_mode) == 'data') then
if (config_use_sgr_opt_kpp == 1) then
if ( trim(config_sgr_flux_vertical_location) == 'top' ) then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to diagnostics, would it make sense to add and else this this and raise an error if config_sgr_flux_vertical_location has an unexpected value? Otherwise, I worry that a typo could run fine but have weird and hard-to-explain behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

<var name="FeSurfaceFluxRunoff" array_group="ecosysSurfaceFluxRunoffSurfaceFluxRunoffGRP" units="mmol Fe m^{-3} m s^{-1}"
<var name="FeSurfaceFluxRunoff" array_group="ecosysSurfaceFluxRunoffGRP" units="mmol Fe m^{-3} m s^{-1}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure this is right but probably should be in its own PR. Do you want to check with Mat Maltrud about that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I told him about it a while ago, but I think he forgot about it

@xylar xylar changed the base branch from master to alternate July 11, 2024 15:30
@xylar xylar changed the base branch from alternate to master July 11, 2024 15:30
@irenavankova
Copy link
Author

I rebased to master and moved to E3SM Project as new branched named ocn/add-subglacial-runoff

xylar pushed a commit that referenced this pull request Sep 12, 2024
…evelop

This PR follows #82, which added subglacial hydrology (SGH) quantities to the
global stats analysis member, by adding SGH quantities to the regional stats
analysis member.

This PR also address a bug introduced in #82 where the calculation of
groundingLineFlux and groundingLineMigrationFlux were moved within an
if config_SGH then condition. The bug prevents groundingLineFlux and
groundingLineMigrationFlux from being calculated by global stats unless
the SGH model is turned on, despite these quantities applying to simulations
where SGH is not used.

* MALI-Dev/andrewdnolan/mali/hydro_regional_stats:
  Fix registry typo Matt caught in review.
  Remove `regionVertexMasks` from registry to minimize file size.
  Apply suggestions from code review
  Use `regionEdgeMask` to calculate SGH regional stats defined on edges.
  Deallocate `regionalSumFlotationFraction`.
  Add missing `regionCellMasks` call from SGH regional stats terms
  Add grounded ice mask to `externalWaterInput` term
  Move reduction of `fluxAcrossGroundingLine` outside SGH condition.
  Add support for subglacial hydro quantities in regional stats.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants