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

First step in adding Tf #9

Closed
wants to merge 4 commits into from
Closed

First step in adding Tf #9

wants to merge 4 commits into from

Conversation

dabail10
Copy link

@dabail10 dabail10 commented Oct 7, 2022

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Short (1 sentence) summary of your PR:
    Added Tf to all the subroutine calls and used in place of Tocnfrz.
  • Developer(s):
    dabail10 (D. Bailey)
  • Suggest PR reviewers from list in the column to the right.
    @eclare108213 @apcraig
  • Please copy the PR test results link or provide a summary of testing completed below.
    Compared two single cases with and without the change.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:
    This is the first step in this change. The next will change answers for the BGC. Note this might also change answers in CICE at least in the history files.

apcraig
apcraig reviewed Oct 7, 2022
View reviewed changes
configuration/driver/icedrv_forcing.F90 Outdated
! 'mushy' Tf conforms with mushy layer thermo (ktherm=2)
! Compute ocean freezing temperature Tf based on tfrz_option
! 'minus1p8' Tf = -1.8 C (default)
! 'constant' Tf = Tocnfrz from namelist
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra space to be removed?

eclare108213
eclare108213 reviewed Oct 7, 2022
View reviewed changes
columnphysics/icepack_mechred.F90 Outdated
@@ -1162,7 +1168,7 @@ subroutine ridge_shift (ntrcr, dt, &

real (kind=dbl_kind), dimension(:), intent(inout) :: &
miso ! isotope mass added to ocean (kg m-2)

Copy link
Collaborator

Choose a reason for hiding this comment

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

extra space added here

configuration/driver/icedrv_forcing.F90 Outdated
! 'linear_salt' Tf = -depressT * sss
! 'mushy' Tf conforms with mushy layer thermo (ktherm=2)
! Compute ocean freezing temperature Tf based on tfrz_option
! 'minus1p8' Tf = -1.8 C (default)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this 'default' comment be changed?

apcraig
apcraig reviewed Oct 7, 2022
View reviewed changes
doc/source/icepack_index.rst Outdated
@@ -454,7 +454,7 @@ either Celsius or Kelvin units).
"tday", "absolute day number", ""
"Tf", "freezing temperature", "C"
"Tffresh", "freezing temp of fresh ice", "273.15 K"
"tfrz_option", ":math:`\bullet` form of ocean freezing temperature", ""
"tfrz_option", ":math:`\bullet` form of ocean freezing temperature", "mushy"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FYI, it's not clear whether the last column is supposed to be the default or units or something else. I think leaving this blank is OK, it's also OK to define the default, or probably to say "character string".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here in the index, I think the convention is to not put default values from the code unless they are hard-coded and/or are not expected to change (like the freezing temperature of fresh ice), but do put units where possible. The default values belong in ug_case_settings, as @apcraig notes below.

apcraig
apcraig reviewed Oct 7, 2022
View reviewed changes
doc/source/user_guide/ug_case_settings.rst Outdated
@@ -359,8 +359,9 @@ forcing_nml
"", "``mm_per_sec``", "(same as MKS units)", ""
"", "``m_per_sec``", "", ""
"``restore_ocn``", "logical", "restore sst to data", "``.false.``"
"``tfrz_option``", "``linear_salt``", "linear functino of salinity (ktherm=1)", "``mushy``"
"``tfrz_option``", "``linear_salt``", "linear function of salinity (ktherm=1)", "``mushy``"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where the default should be defined and it is. Could you reorder the options into alphabetical order. I know it's a pain, but that means constant would be first. Thanks!

eclare108213
eclare108213 reviewed Oct 7, 2022
View reviewed changes
doc/source/user_guide/ug_case_settings.rst Outdated
@@ -359,7 +359,8 @@ forcing_nml
"", "``mm_per_sec``", "(same as MKS units)", ""
"", "``m_per_sec``", "", ""
"``restore_ocn``", "logical", "restore sst to data", "``.false.``"
"``tfrz_option``", "``linear_salt``", "linear functino of salinity (ktherm=1)", "``mushy``"
"``tfrz_option``","``constant``", "constant ocean freezing temperature (Tocnfrz)"
"", "``linear_salt``", "linear function of salinity (ktherm=1)", "``mushy``", ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an extra pair of quotation marks in this line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "mushy" default value needs to be moved to the "constant" line then I think it's OK.

eclare108213
eclare108213 requested changes Oct 10, 2022
View reviewed changes
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 is set to be merged into E3SM-Project:main, not what we want to do! It needs to be a PR into E3SM-Project:cice-consortium/E3SM-icepack-initial-integration

@dabail10
Copy link
Author

Is anyone else allowed to edit the destination? Or do I need to close and re-open?

@eclare108213 eclare108213 changed the base branch from main to cice-consortium/E3SM-icepack-initial-integration October 10, 2022 22:58
@eclare108213
Copy link
Collaborator

I was able to change it. But now there are conflicts...

@apcraig
Copy link
Collaborator

apcraig commented Oct 10, 2022

@eclare108213, good catch!

@dabail10
Copy link
Author

Ok. I will fix this.

@apcraig
Copy link
Collaborator

apcraig commented Oct 10, 2022

I don't understand the conflicts. Not sure why they're showing up. @dabail10, please review the PR diffs to make sure you didn't start from a different version of the code. All the diffs should be recognizable to you.

@dabail10
Copy link
Author

I had to do a rebase. I guess main is different enough from this branch?

@dabail10
Copy link
Author

Looks like I have to do this another way.

@dabail10
Copy link
Author

I tried to merge the branch into my branch tfrz. It was requiring a rebase and then would not let me push it back to tfrz. I was able to create a new branch tfrz2 from the E3SM-Icepack-partial-integration. Then I could merge from tfrz to here. I will close this and reissue from tfrz2.

@dabail10 dabail10 closed this Oct 10, 2022
@apcraig
Copy link
Collaborator

apcraig commented Oct 10, 2022

OK, so you didn't start with E3SM-Project:cice-consortium/E3SM-icepack-initial-integration? I'm surprised the PR is working as well as it is then. For reference, this is what I do to develop the E3SM Icepack.

git clone https://github.com/E3SM-Project/Icepack icepack.mybranch
cd icepack.mybranch
git checkout cice-consortium/E3SM-icepack-initial-integration
git branch mybranch
git checkout mybranch
git remote add myfork https://github.com/apcraig/Icepack
# develop and test
git commit ...
git push myfork mybranch

I start with the branch I want to work on, create a local branch and develop there. I then push the branch onto my fork. Then PR from myfork:mybranch to E3SM-Project:cice-consortium/E3SM-icepack-initial-integration. I think this is the easiest way to contribute on non-standard forks/branches.

In fact, we maybe should be doing that by default even for the Consortium main. You need not keep your fork/branch up to date with main and branch off your fork main. You can just checkout CICE-Consortium:main, create a branch, develop, and then finally push back to your fork. Then PR back to the Consortium. The only drawback to that you have to be very careful not to do "git push origin mybranch" at the end or you push directly to CICE-Consortium which we don't ever want.

@dabail10
Copy link
Author

The tfrz branch had started from main. This one I started from the correct branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apcraig apcraig apcraig left review comments

@eclare108213 eclare108213 eclare108213 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dabail10 @eclare108213 @apcraig