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

Code to read new bathymetry files for basal stress parameterization. #222

Merged
merged 8 commits into from
Nov 6, 2018
Merged

Conversation

dabail10
Copy link
Contributor

This just adds code to read in new bathymetry files. By default, this is turned off (use_bathymetry = .false. and basalstress = .false.) There are two new files needed for inputdata which I will add shortly. I have added use_bathymetry = .true. to alt01 and alt03.

  • Developer(s): D. Bailey, J. F. Lemieux, F. Dupont

  • Please suggest code Pull Request reviewers in the column at right.

  • Are the code changes bit for bit, different at roundoff level, or more substantial? BFB for non basal stress cases. The answers will change when use_bathymetry is .true. and the new bathymetry files are read in. So, the answers for alt01 and alt03 will be different.

  • Please include the link to test results or paste the summary block from the bottom of the testing output below.

  • Does this PR create or have dependencies on Icepack or any other models?

  • Is the documentation being updated with this PR? (Y/N) N
    If not, does the documentation need to be updated separately at a later time? (Y/N) Y

Note: "Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.

  • Other Relevant Details:

@dabail10
Copy link
Contributor Author

Update documentation.

Copy link
Contributor

@apcraig apcraig 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 OK to me technically. When the new datasets are available, I will update some machines and run some tests to confirm the combination of the code mods and new datasets are working. Please update this PR when the input data is updated.

Copy link
Contributor

@JFLemieux73 JFLemieux73 left a comment

Choose a reason for hiding this comment

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

Fred Dupont and I just had a look at this. Looks good.

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.

Looks good to merge when the new files are available on ftp and @apcraig has tested it. Thanks!

@mhrib
Copy link
Contributor

mhrib commented Nov 1, 2018

Was about to suggest similar changes ... but you were faster:-)

In the file: ice_grid.F90 --> subroutine read_basalstress_bathy
I would suggest to remove the following write statements:
write (,) 'Initial ice file: ', trim(init_file)
write (,) 'Bathymetry file: ', trim(bathymetry_file)
as they are redundant using write(nu_diag,*)

rgs
Mads

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

I added the datasets to the FTP area and it looks like all tests have passed.

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

I am testing on conrad now and will pull the new input data on gordon and thunder as well. Have the input data areas on hobart and cheyenne been updated?

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

It is on cheyenne, but not hobart yet. Doing that shortly.

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

Tests are still running on conrad, but it looks like alt03 is not bit-for-bit but alt01 (and all other configurations) are bit-for-bit. This seems inconsistent with the info in the PR submission which indicated both alt01 and alt03 should change answers. thoughts?

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

@dabail10, what does "all tests have passed" mean. What machine/compiler and did you do any regression testing?

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

Interesting. Let me look into this a bit.

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

I meant the Travis Suite.

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

Here are the test results on conrad, hash a5364f0, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks. The one compare fail is alt03 which is expected.

Once Dave clarifies the alt01 results, I think we can merge, assuming no other changes are needed.

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

Perhaps @JFLemieux73 needs to chime in here, but alt01 is a gx3 case. I noticed that the most shallow water column is 31 m. Is this the same as in the POP kmt file for gx3? Looks like the KMT has 3 levels in the Arctic and hence 30m depth. So, does this mean that basalstress does nothing in the gx3? I'm also unsure about the precedence of the set_nml files. Does alt01 take precedence over gx3?

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

Regarding the precedent of the options files. It's basically alphabetical with files that start later in the alphabet having precedent. This is not ideal, but hoping that conflicts between files are minimal. If that is not working, we need to discuss how to implement options precedence in a robust way. An early implementation of the options had individual namelist separated into their own files and then layering on top of that such that we could get exactly what we wanted. But that's a bit more complex. We may need to go back to something like that. Another way is to not allow multiple options in a single test. That might mean we end up with gx3alt01 and gx1alt01 as two different and separate options settings.

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

Ok. So, it sounds like I need to remove use_bathymetry from the set_nml.gx3 and gx1 files. It is set to false internally in ice_init.F90, but I thought I should make sure the default for the tests other than alt01 and alt03 would be use_bathymetry = .false.

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

It looks like you also need to remove the bathymetry_file setting from the alt01 and alt03 set_nml files. You are hardwiring to gx3 there which is not what you want.

@JFLemieux73
Copy link
Contributor

Hi,

Sorry for the delay...

I just opened the gx3_bathy.nc with ncview. The displayed range is 31 m to 5468 m (confusing...) but there are smaller values than 31 m (as small as 10 m). So grounding should occur and we should expect diff with the kmt approach.

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

Good point. I have removed it now.

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

I will run a subset of tests on conrad to see where we're at now.

@apcraig
Copy link
Contributor

apcraig commented Nov 1, 2018

alt01 is still bit-for-bit with master. alt03 still is not. They are both gx3 tests.
conrad_intel_restart_gx3_6x2_alt01
conrad_intel_restart_gx3_4x2_alt03
Can you setup these tests independently and just make sure the namelist are as expected and they are doing what we think they should.

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 1, 2018

Ok. I will try these tests.

@dabail10
Copy link
Contributor Author

dabail10 commented Nov 6, 2018

This works as expected. Two tests have basalstress on and use_bathymetry on. The one test, alt03 has different answers as expected. The other test, alt01 is bfb. This is because kdyn = 0. Not sure why alt01 has kdyn = 0 and basalstress = .true. Otherwise, this is ready to go.

@apcraig apcraig merged commit 8b5d76a into CICE-Consortium:master Nov 6, 2018
@duvivier duvivier mentioned this pull request Nov 7, 2018
@dabail10 dabail10 deleted the bathy branch June 12, 2019 16:52
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