-
Notifications
You must be signed in to change notification settings - Fork 4
add optional wave height input to subroutines in icepack that calculate wave heights #40
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 optional wave height input to subroutines in icepack that calculate wave heights #40
Conversation
eclare108213
left a comment
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 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?
proteanplanet
left a comment
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.
Assuming changes recommended by @eclare108213 are enacted, I approve this PR.
@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? |
|
@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. |
eclare108213
left a comment
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.
Thanks for updating the icepack driver too. Did you run icepack tests in standalone mode? If not, I can help ...
|
There is also some documentation that needs to be updated here: doc/source/science_guide/sg_itd.rst |
|
Are you doing something to change this line? |
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. |
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? |
|
@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) |
eclare108213
left a comment
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.
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.
|
@eclare108213 - ready for standalone testing. I am testing new icepack namelist settings in the coupled e3sm setup. |
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.
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
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.