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

Subroutines for CD-grid rheology #10

Merged
merged 4 commits into from
Nov 16, 2021
Merged

Subroutines for CD-grid rheology #10

merged 4 commits into from
Nov 16, 2021

Conversation

JFLemieux73
Copy link
Collaborator

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:
    ENTER INFORMATION HERE
  • Developer(s):
    ENTER INFORMATION HERE
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    ENTER INFORMATION HERE
  • 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:

@phil-blain
Copy link
Collaborator

supersedes #9

@phil-blain phil-blain mentioned this pull request Nov 16, 2021
Closed
16 tasks
@phil-blain phil-blain changed the title Subroutines for Cdgrid rheology Subroutines for CD-grid rheology Nov 16, 2021
phil-blain
phil-blain reviewed Nov 16, 2021
View reviewed changes
cicecore/cicedynB/dynamics/ice_dyn_shared.F90
Comment on lines +1291 to +1300
subroutine deformations_T (nx_block, ny_block, &
icellt, &
indxti, indxtj, &
uvelE, vvelE, &
uvelN, vvelN, &
dxN, dyE, &
dxT, dyT, &
tarear, &
shear, divu, &
rdg_conv, rdg_shear )
Copy link
Collaborator

@phil-blain phil-blain Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This whole subroutine looks like it's the same as deformations, only calling strain_ratesT instead of strain_rates.

Should we instead keep the existing deformations subroutine, and add an if (grid_system == 'B') .. else .. block for calling strain_rates vs strain_ratesT ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good eyes, @phil-blain .
I would prefer to reuse as much code as possible. Initially it's fine to have duplicative code, while developing, but if it makes sense and is easy to merge/clean up enroute, let's do it...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, upon further inspection with @JFLemieux73, it's not exactly the same, as the B grid version does the average of the 4 corners:
https://github.com/CICE-Consortium/CICE/blob/0f75f068d63725062da4ea944443fbdd6dbed2fb/cicecore/cicedynB/dynamics/ice_dyn_shared.F90#L1269-L1278

whereas the CD-grid version already has the quantities at the tracer point, so it just computes the deformations without averaging:
https://github.com/CICE-Consortium/CICE/blob/0f75f068d63725062da4ea944443fbdd6dbed2fb/cicecore/cicedynB/dynamics/ice_dyn_shared.F90#L1363-L1370

@apcraig apcraig merged commit e9aa132 into apcraig:cgridDEV Nov 16, 2021
@apcraig
Copy link
Owner

apcraig commented Nov 16, 2021

Sorry, merged before I realized there was some discussion. I can unmerge, but propose we consider @phil-blain suggestion and create another PR if we want to make some changes.

@phil-blain
Copy link
Collaborator

@JFLemieux73 what do you think?

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

@phil-blain phil-blain phil-blain left review comments

@apcraig apcraig apcraig approved these changes

@dabail10 dabail10 Awaiting requested review from dabail10

@daveh150 daveh150 Awaiting requested review from daveh150

@eclare108213 eclare108213 Awaiting requested review from eclare108213

@rallard77 rallard77 Awaiting requested review from rallard77

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

Successfully merging this pull request may close these issues.
4 participants
@JFLemieux73 @phil-blain @apcraig @eclare108213