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

Express rheology term as a function of viscous coefficients for EVP and rEVP #639

Merged
merged 10 commits into from
Oct 8, 2021

Conversation

JFLemieux73
Copy link
Contributor

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:
    Express rheology term as a function of viscous coefficients for EVP and rEVP
  • Developer(s):
    @JFLemieux73
  • Suggest PR reviewers from list in the column to the right. @eclare108213
  • Please copy the PR test results link or provide a summary of testing completed below.
    see Express rheology term as a function of viscous coefficients for EVP and rEVP #634
  • 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 Icepack 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/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

@JFLemieux73 JFLemieux73 changed the title Visc coeff Express rheology term as a function of viscous coefficients for EVP and rEVP Oct 6, 2021
@@ -640,9 +642,10 @@ subroutine stress (nx_block, ny_block, &
tensionne, tensionnw, tensionse, tensionsw, & ! tension
shearne, shearnw, shearse, shearsw , & ! shearing
Deltane, Deltanw, Deltase, Deltasw , & ! Delt
zetane, zetanw, zetase, zetasw , & ! zeta viscous coeff
etane, etanw, etase, etasw , & ! eta viscous coeff
Copy link
Member

Choose a reason for hiding this comment

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

it would be nicer if this line was aligned correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

@@ -202,6 +202,7 @@ either Celsius or Kelvin units).
"eps13", "a small number", "10\ :math:`^{-13}`"
"eps16", "a small number", "10\ :math:`^{-16}`"
"esno(n)", "energy of melting of snow per unit area (in category n)", "J/m\ :math:`^2`"
"eta", "viscous coefficient (shear)", "kg/s"
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to point out here that this is actually 2*eta ? (and same for zeta below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will think about it a bit more and might correct it later in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's confusing to use eta and zeta, especially if they are different from the 'classic' definitions. Maybe etax2 and zetax2? Or eta2 and zeta2, although those could also be ^2 rather than x2. The code isn't consistent with this kind of thing.

@phil-blain
Copy link
Member

@phil-blain
Copy link
Member

Also (not really related), I'm noticing this link: https://github.com/CICE-Consortium/CICE/pull/639/files#diff-11d70aaccc7bcc5a2940f34bce5510966aa84c7cedbb6d59df0a1f61f1945d28R387-R388 does not work, which could be corrected while we're at it...

@phil-blain
Copy link
Member

phil-blain commented Oct 6, 2021

oups my link does not work (GitHub bug it seems when linking to an expanded portion of the diff). I'm talking about the link to the Icepack doc at the end of https://github.com/CICE-Consortium/CICE/blob/85d7b71840cfecb6d12fd1d74270a340ac2c23f6/doc/source/science_guide/sg_dynamics.rst#internal-stress

@apcraig
Copy link
Contributor

apcraig commented Oct 6, 2021

I'm talking about the link to the Icepack doc at the end of https://github.com/CICE-Consortium/CICE/blob/85d7b71840cfecb6d12fd1d74270a340ac2c23f6/doc/source/science_guide/sg_dynamics.rst#internal-stress

It looks/works OK in the readthedocs to me. Maybe we're looking at different things?

@apcraig apcraig requested a review from eclare108213 October 6, 2021 17:21
Copy link
Contributor

@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 good to me, except the eta and zeta variable names. Open to suggestions.

@@ -202,6 +202,7 @@ either Celsius or Kelvin units).
"eps13", "a small number", "10\ :math:`^{-13}`"
"eps16", "a small number", "10\ :math:`^{-16}`"
"esno(n)", "energy of melting of snow per unit area (in category n)", "J/m\ :math:`^2`"
"eta", "viscous coefficient (shear)", "kg/s"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's confusing to use eta and zeta, especially if they are different from the 'classic' definitions. Maybe etax2 and zetax2? Or eta2 and zeta2, although those could also be ^2 rather than x2. The code isn't consistent with this kind of thing.

@JFLemieux73
Copy link
Contributor Author

I like etax2 and zetax2...I will make these changes.

@JFLemieux73
Copy link
Contributor Author

So I just changed eta,zeta to etax2,zetax2. I have verified that I did not break anything. I ran gx3 for 1 day and the result with etax2,zetax2 is BFB with respect to the result with eta,zeta (for both evp and revised evp).

Copy link
Contributor

@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.

I realize that I'm not being consistent in my complaints about variable names. I usually ask not to use greek letters, preferring something more descriptive. In this case, that would be something like viscoshearx2, but eta and zeta are so entrenched from the Hibler79 VP model, I don't mind in this case. Just saying. Happy to entertain other opinions but will approve for now. If we don't hear anything by end-of-day tomorrow, we'll merge for weekend testing.

@JFLemieux73
Copy link
Contributor Author

Philippe also asked me to confirm that all the tests that "failed" in the end-to-end procedure are the ones with EVP and revised EVP. It is the case: the tests with VP, EAP and 1d-EVP (eta and zeta are not used yet for 1d-EVP) are BFB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants