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

Reorder calls to neutral_Coefficients and frzmlt_bottom_lateral - bugfix #380

Closed
wants to merge 0 commits into from

Conversation

TillRasmussen
Copy link
Contributor

@TillRasmussen TillRasmussen commented Dec 7, 2021

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:
    The order of calls to neutral_drag_coeffs and frzmlt_bottom_lateral changed in order to use Cdn_ocn from current timestep when formdrag is true and fbot_xfer_type = 'Cdn_ocn'
  • Developer(s):
    @TillRasmussen
  • [ x] Suggest PR reviewers from list in the column to the right.
  • @eclare108213 @proteanplanet @apcraig @dabail10
  • Please copy the PR test results link or provide a summary of testing completed below.
    Not bit for bit when formdrag= true and xfer_type=Cdn_ocn. It passes the QC test
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • [x ] more substantial
  • Does this PR create or have dependencies on CICE or any other models?
    • [ x] Yes (new icepack version within CICE)
    • No
  • Does this PR add any new test cases?
    • Yes
    • [x ] 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
    • [ x] No, does the documentation need to be updated at a later time?
      • Yes
      • [x ] No
  • Please provide any additional information or relevant details below:

@phil-blain
Copy link
Member

Hi @TillRasmussen, the title of the PR becomes the title of the commit message when the PR is merged. Would it be possible to use a more explicit title that describes what the PR is doing in 50-70 characters ? thanks.

@TillRasmussen TillRasmussen changed the title Main Reorder calls to neutral_Coefficients and frzmlt_bottom_lateral - bugfix Dec 7, 2021
@TillRasmussen
Copy link
Contributor Author

TillRasmussen commented Dec 7, 2021

Hi @TillRasmussen, the title of the PR becomes the title of the commit message when the PR is merged. Would it be possible to use a more explicit title that describes what the PR is doing in 50-70 characters ? thanks
Done. I did notice it by my time was allocated elsewhere all day.

@apcraig
Copy link
Contributor

apcraig commented Dec 8, 2021

Curious why the prior commit, #378 is showing up in this PR.

It looks like only the "alt04" tests should be affected. We'll want to check that once we merge and run the full suite.

@phil-blain
Copy link
Member

Curious why the prior commit, #378 is showing up in this PR.

It's not the previous commit on main, but rather a broken copy of that commit (probably created by a cancelled merge from main). If you checkout this pull request locally, and check the commit graph, we see this:

$ git fetch https://github.com/cice-consortium/icepack.git pull/380/head:pr/380
$ git log --oneline --decorate --graph -10 pr/380 upstream/main 
* df1d3ed Added method to check namelist errors (#379) daveh150 (24 minutes ago)  (upstream/main)
* b7148ee Fix two output conversion errors  (#378) Philippe Blain (Thu Dec 2 14:43) 
| * 5d19492 change order of calls to neutral_drag and frzmlt. Fix bug TillRasmussen (Tue 08:24 +0000)  (HEAD -> pr/380, upstream/pr/380)
| * d48640a remove old url Till Rasmussen (Oct 23 2020) 
| * 54125aa Fix two output conversion errors  (#378) Philippe Blain (Thu Dec 2 14:43) 
|/  
* 152bd70 protect merge of meltsliq when tr_snow is off (#376) Tony Craig (Fri Nov 12 10:41) 

And the diff for commit 54125aa contains a non-removed conflict marker, which is later removed in d48640a.

I think it would be better to rebase the branch on top of an updated mainand keep only the tip commit
5d19492.

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

Successfully merging this pull request may close these issues.

4 participants