Skip to content

Conversation

@erinethomas
Copy link

This PR adds an optional wave height input parameter to the wave fracture subroutine, that allows it to use the wave height from a coupled wave model as opposed to Icepack calculating its own wave height.
This change has been tested in fully coupled E3SM simulations with active wave-sea ice coupling.

@erinethomas erinethomas changed the title add optional wave height input to wavefracspec add optional wave height input to subroutines in icepack that calculate wave heights Jul 30, 2025
@erinethomas erinethomas requested review from eclare108213 and removed request for eclare108213 October 8, 2025 16:58
Copy link
Collaborator

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

This looks fine to me. Since wave_height is added as an optional argument in an Icepack interface routine, then I believe its status (present or not) will be inherited in other subroutine calls following this one, and if the driver does not include it, the code should be backward compatible. But since optional arguments continue to befuddle me, I'd like @apcraig to confirm that the use of optional and if (present()) is correct in this PR (thank you).

When this change is pulled back to the Consortium repo, there will need to be a change to the Icepack and CICE drivers to recognize the new option using a namelist flag. @erin do you have a flag for it in E3SM, or is it somehow hard-coded as part of the LRW compset?

@erinethomas erinethomas requested a review from apcraig October 8, 2025 20:26
Copy link

@proteanplanet proteanplanet left a comment

Choose a reason for hiding this comment

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

Assuming changes recommended by @eclare108213 are enacted, I approve this PR.

@erinethomas
Copy link
Author

This looks fine to me. Since wave_height is added as an optional argument in an Icepack interface routine, then I believe its status (present or not) will be inherited in other subroutine calls following this one, and if the driver does not include it, the code should be backward compatible. But since optional arguments continue to befuddle me, I'd like @apcraig to confirm that the use of optional and if (present()) is correct in this PR (thank you).

When this change is pulled back to the Consortium repo, there will need to be a change to the Icepack and CICE drivers to recognize the new option using a namelist flag. @erin do you have a flag for it in E3SM, or is it somehow hard-coded as part of the LRW compset?

@eclare108213 - as an optional variable, if the wave height is not provided, the code runs as it currently does: by calculating the wave height. no namelist setting is needed to control it. In E3SM, we do have a driver namelist flag to control wave-ice coupling based on the compset selected by the user. All wave - sea ice coupling variables are controlled by this one namelist setting, the variables do not have individual namelist settings. In other words, it is not possible to pass the wave spectra and NOT the wave height in the E3SM setup, if you have an wave model, all wave variables are calculated by the wave model and passed to MPAS-SI. Hope this answers your question.

@eclare108213
Copy link
Collaborator

This looks fine to me. Since wave_height is added as an optional argument in an Icepack interface routine, then I believe its status (present or not) will be inherited in other subroutine calls following this one, and if the driver does not include it, the code should be backward compatible. But since optional arguments continue to befuddle me, I'd like @apcraig to confirm that the use of optional and if (present()) is correct in this PR (thank you).
When this change is pulled back to the Consortium repo, there will need to be a change to the Icepack and CICE drivers to recognize the new option using a namelist flag. @erin do you have a flag for it in E3SM, or is it somehow hard-coded as part of the LRW compset?

@eclare108213 - as an optional variable, if the wave height is not provided, the code runs as it currently does: by calculating the wave height. no namelist setting is needed to control it. In E3SM, we do have a driver namelist flag to control wave-ice coupling based on the compset selected by the user. All wave - sea ice coupling variables are controlled by this one namelist setting, the variables do not have individual namelist settings. In other words, it is not possible to pass the wave spectra and NOT the wave height in the E3SM setup, if you have an wave model, all wave variables are calculated by the wave model and passed to MPAS-SI. Hope this answers your question.

As a PR into E3SM, this behavior is fine, but for other users there will need to be a namelist option added to control it, since the default at the moment is to pass one thing and not the other (spectra, height). Having important behavior like this controlled only by the presence of an optional argument is a little risky, not good coding practice in my opinion. My preference would be to go ahead and add the namelist (and associated documentation) to Icepack now. I'm willing to do that and PR it into your branch. Then you could choose whether to add it as a namelist in MPAS-SI or simply hardwire it when you initialize Icepack's parameters. Would that be okay?

@erinethomas
Copy link
Author

@eclare108213 - yes, I will add a namelist option so the lines of code that check for the existence of a passed wave height field use both present(wave_height) AND a namelist flag. Code modification coming soon.

Copy link
Collaborator

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

Thanks for updating the icepack driver too. Did you run icepack tests in standalone mode? If not, I can help ...

@eclare108213
Copy link
Collaborator

There is also some documentation that needs to be updated here: doc/source/science_guide/sg_itd.rst

@eclare108213
Copy link
Collaborator

eclare108213 commented Nov 6, 2025

Are you doing something to change this line?
configuration/driver/icedrv_step.F90
Edit: remove question re E3SM -- this IS the E3SM fork of Icepack.

@erinethomas
Copy link
Author

erinethomas commented Nov 12, 2025

Are you doing something to change this line? configuration/driver/icedrv_step.F90 Edit: remove question re E3SM -- this IS the E3SM fork of Icepack.

I am not changing this line - since E3SM version of the code does NOT use the icepack driver (I have modified the driver used in the E3SM version) however, people using the stand alone/icepack driver probably will need to modify this if they try to run icepack coupled to a wave model? Happy to discuss the options needed here for stand alone runs.

@erinethomas
Copy link
Author

erinethomas commented Nov 12, 2025

There is also some documentation that needs to be updated here: doc/source/science_guide/sg_itd.rst

I do not see any issues with this documentation. The way significant wave height is calculated (internally or passed from a wave model) does not impact the way the FSD gets updated (from breaking). How exactly would you like me to update this documentation?

@erinethomas
Copy link
Author

@eclare108213 - I have addressed most of your comments- a second round of review would be appreciated! (Also I may need help testing this in a stand alone icepack case)

Copy link
Collaborator

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

All my suggestions here are cosmetic except how to handle the wave_height_type='none' case. That needs to be done a little differently to maintain our current tests. If you'll commit this then I'll set up a standalone Icepack test suite to double check things are working as expected.

@erinethomas
Copy link
Author

erinethomas commented Nov 13, 2025

@eclare108213 - ready for standalone testing. I am testing new icepack namelist settings in the coupled e3sm setup.

Copy link
Collaborator

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

endif needs to be added at line 666 in icedrv_init.F90. With that change, base_suite tests all pass, including those with fsd turned on:

215 of 215 tests PASSED
0 of 215 tests PENDING
0 of 215 tests MISSING data
0 of 215 tests FAILED

testsuite.e3sm_waveht eclare$ ./results.csh | grep fsd
PASS conda_macos_smoke_col_1x1_debug_fsd12_run1year_short build
PASS conda_macos_smoke_col_1x1_debug_fsd12_run1year_short run
PASS conda_macos_smoke_col_1x1_debug_fsd12_run1year_short test
PASS conda_macos_smoke_col_1x1_debug_fsd12_run1year_short compare e3sm_base
PASS conda_macos_smoke_col_1x1_debug_fsd1_run1year build
PASS conda_macos_smoke_col_1x1_debug_fsd1_run1year run
PASS conda_macos_smoke_col_1x1_debug_fsd1_run1year test
PASS conda_macos_smoke_col_1x1_debug_fsd1_run1year compare e3sm_base
PASS conda_macos_restart_col_1x1_fsd12_short build
PASS conda_macos_restart_col_1x1_fsd12_short initialrun
PASS conda_macos_restart_col_1x1_fsd12_short run 
PASS conda_macos_restart_col_1x1_fsd12_short test 
PASS conda_macos_restart_col_1x1_fsd12_short compare e3sm_base

For the record, here's what I did to run these these tests:

create baseline:

git clone https://github.com/E3SM-Project/icepack icepack_e3sm_20251116
cd icepack_e3sm_20251116
./icepack.setup --suite base_suite -m conda -e macos -p 1x1 --testid e3sm_base --bgen e3sm_base 

clone wave code and test against baseline:

git clone https://github.com/erinethomas/icepack -b optional_WaveHeight icepack_e3sm_waveht
cd icepack_e3sm_waveht
./icepack.setup --suite base_suite -m conda -e macos -p 1x1 --testid e3sm_waveht --bcmp e3sm_base 

@darincomeau darincomeau merged commit 7e35af5 into E3SM-Project:main Nov 18, 2025
1 check failed
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.

4 participants