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.

Sign up for GitHub

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

Jump to bottom

Awicm3 ice thermo cpl fixes #93

Merged
merged 6 commits into from
Apr 12, 2021
Merged

Awicm3 ice thermo cpl fixes #93

merged 6 commits into from
Apr 12, 2021

Conversation

JanStreffing
Copy link
Collaborator

This branch is the result of extensive check of the sea ice coupling by Tido, Qiang, Dima Sidorenko, Wolfgang Dorn and others. The changes are confined to routines that are called when FESOM2 is coupled to OpenIFS.

The discussion behind this branch took place in https://github.com/AWI-CM3/project_digital_twin/issues/14

@JanStreffing JanStreffing added the bug Something isn't working label Apr 10, 2021
koldunovn
koldunovn approved these changes Apr 10, 2021
View reviewed changes
Copy link
Member

@koldunovn koldunovn left a comment

Choose a reason for hiding this comment

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

I am fine with it, have only one comment, that just a suggestion, feel free to consider or ignore it :) I would wait for at least another review before merge.

src/ice_thermo_cpl.F90
! h0max = 0.75
! endif
!#endif /* (__oifs) */
#if defined (__oifs)
Copy link
Member

Choose a reason for hiding this comment

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

We had before the situation, when people where trying to optimize for h0, available in the namelist, but then hardcoded downstream. Will we get into similar trap here? Should we maybe expose those parameters explicitly through namelist.ice and comment that they only used in a coupled setting?

Copy link
Collaborator

Choose a reason for hiding this comment

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

great job, great bug fixes! technically it can be merged as is. @koldunovn shall we introduce namelist.cpl or a section &cpl in one of the namelists for FESOM?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe not for this PR, but later I think it's not a bad idea. If you OK with this PR I would merge as is.

Copy link
Collaborator Author

@JanStreffing JanStreffing Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would be ok with that. And to have the namelist.cpl be a separate development.

@koldunovn koldunovn merged commit 34cfaba into master Apr 12, 2021
wiltonloch pushed a commit to wiltonloch/fesom2 that referenced this pull request Feb 1, 2023
@koldunovn
Merge pull request FESOM#93 from FESOM/awicm3-ice_thermo_cpl_fixes
00c8966 
AWI-CM3 ice_thermo_cpl fixes
@suvarchal suvarchal deleted the awicm3-ice_thermo_cpl_fixes branch December 13, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koldunovn koldunovn koldunovn approved these changes

@dsidoren dsidoren Awaiting requested review from dsidoren

@patrickscholz patrickscholz Awaiting requested review from patrickscholz

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JanStreffing @koldunovn @dsidoren