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

Moved floediam and hfrazilmin to icepack_parameters #342

Merged
merged 6 commits into from
Nov 24, 2020

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Nov 21, 2020

PR checklist

  • Short (1 sentence) summary of your PR:
    Moved floediam and hfrazilmin to icepack parameters
  • Developer(s):
    apcraig
  • 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.
    Tested on cheyenne with 3 compilers. fsd results are different. We updated the floeshape value to 0.66 and those test results are
    https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#d94b1d60f7e53c42591da3aa73254133083a96d5
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
      fixed duplicate definition of floeshape and changed working value from 0.666 to 0.66.
  • 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:

See CICE-Consortium/CICE#472
Added floediam and hfrazilmin to the thermo namelist in the icepack driver
Settable via a call to icepack_parameters
Defaults left unchanged
Added variables to icepack_in
Removed redundant declaration of floeshape in icepack_therm_vertical. This resulted in non bit-for-bit results for fsd cases. effective floeshape was changed from 0.666 to 0.66.

Added floediam and hfrazilmin to the thermo namelist in the icepack driver
Settable via a call to icepack_parameters
Defaults left unchanged
Added variables to icepack_in
@apcraig
Copy link
Contributor Author

apcraig commented Nov 21, 2020

The new namelist were added to thermo_nml. If that's not the right namelist group, let me know and I'm happy to change it.

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.

hfrazilmin definitely belongs in the thermo namelist. floediam belongs with the floe size distribution, and I don't see any FSD parameters in the namelist, so thermo seems to be the best place for floediam. However, in looking for FSD parameters, I noticed the floeshape is in two places with different values! I believe that's a bug, and it will change the answers for some tests. It looks like we pulled it out to icepack_parameters but neglected to remove it from the thermo module (or chose to leave the old value in there). floediam and floeshape should appear next to each other when declared and documented, in my opinion.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 21, 2020

I'm happy to fix these things. Just to clarify, @eclare108213,

  • I will remove the definition of floeshape in icepack_therm_vertical and have it reference the value in icepack_parameters. This will change the results slightly for some cases.
  • I will place the declaration of floeshape and floediam next to each other in icepack_parameters under the "parameters for the floe size distribution".
  • When we add floediam and hfrazilmin to namelist in CICE, they will be in the fsd and thermo namelist respectively. Once this is PR is merged to Icepack, I will do that in CICE.
  • Do we want to add an fsd namelist in the icepack driver? If not, I will leave floedam in the thermo namelist in the icepack driver. I don't know if it's helpful to keep the icepack driver and CICE namelist consistent or not.

@eclare108213
Copy link
Contributor

Neither CICE nor Icepack have a namelist for FSD physics parameters - the one in CICE is only history variables (unless I'm just missing it in my search). So I think both floediam and hfrazilmin can go in the thermo namelist.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 21, 2020

Got it. Thanks @eclare108213.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 22, 2020

I updated, tested, and pushed the redundant setting of floeshape. The new results are

https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#bc252dc0652954efa03d90438a9edd71c16efeb2

and as expected, several results are NOT bit-for-bit. I have update the PR documentation.

@eclare108213
Copy link
Contributor

It makes sense that the only ones that are BFB are the FSD tests. Perhaps it would make more sense to keep the default value that we've been using? Then I'd expect the FSD tests to fail but all the others should remain BFB.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 23, 2020

@eclare108213, that makes sense to me. So I'll change the value of floeshape to 0.66 (from 0.666) in icepack_parameters and retest, more soon. If that doesn't seem right, just let me know. And we'd keep 0.66 as the value permanently or is this just to confirm that answers are changing only because of the floeshape value? Either way is OK by me. I can change the value, test, and change it back to 0.666. Or we can leave it at 0.66.

@eclare108213
Copy link
Contributor

@lettie-roach
Do you have a particular preference for the floeshape setting in Icepack and CICE? We can use either 0.66 or 0.666. I'm not sure why we have two slightly different values, but I'm happy to defer to your opinion.

@apcraig
Copy link
Contributor Author

apcraig commented Nov 23, 2020

I update floeshape to 0.66 and the test results are here,

https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#d94b1d60f7e53c42591da3aa73254133083a96d5

As expected, everything is bit-for-bit with the current master except the fsd cases.

Just waiting to hear whether we want floeshape=0.66 or 0.666 as the default value.

@lettie-roach
Copy link
Contributor

lettie-roach commented Nov 23, 2020 via email

@eclare108213
Copy link
Contributor

Let's go with 0.66. I had to track down the Steele reference, which we don't have in our docs:
Steele, M. (1992), Sea ice melting and floe geometry in a simple ice‐ocean model, J. Geophys. Res., 97( C11), 17729– 17738, doi:10.1029/92JC01755.
He uses 0.66, and I don't know where the 0.666 came from. Could track it down, but I'm inclined to just fix this and move on.

@apcraig apcraig merged commit 77c523e into CICE-Consortium:master Nov 24, 2020
apcraig added a commit to apcraig/CICE that referenced this pull request Dec 2, 2020
Add floediam and hfrazilmin as CICE namelist.  Requires latest
version of Icepack.  Update documentation.

Icepack has two non bit-for-bit changes including
  - a fix to multiple declarations of floeshape (CICE-Consortium/Icepack#342)
  - a fix to the convergence scheme in icepack_atmo (CICE-Consortium/Icepack#341)
eclare108213 pushed a commit to CICE-Consortium/CICE that referenced this pull request Dec 3, 2020
Add floediam and hfrazilmin as CICE namelist.  Requires latest
version of Icepack.  Update documentation.

Icepack has two non bit-for-bit changes including
  - a fix to multiple declarations of floeshape (CICE-Consortium/Icepack#342)
  - a fix to the convergence scheme in icepack_atmo (CICE-Consortium/Icepack#341)
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.

5 participants