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

Revised EVP updated #184

Closed
wants to merge 3 commits into from
Closed

Revised EVP updated #184

wants to merge 3 commits into from

Conversation

mhrib
Copy link
Contributor

@mhrib mhrib commented Sep 15, 2018

Revised EVP parameters, according to Kimmirtz, Danilov and Losch, 2015

  • Developer(s): Till Rasmussen and Mads Hvid Ribergaard, DMI

  • Please suggest code Pull Request reviewers in the column at right.
    Elisabeth?, Tony?

  • Are the code changes bit for bit, different at roundoff level, or more substantial?
    With conservative settings - AND revised_evp=.false. - it should output Bit-for-bit identical.
    With revised_evp=.true. the settings has changed. Therefore obvious NOT bit-to-bit identical

  • 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) ??
    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:

affected files:
./cicecore/cicedynB/dynamics/ice_dyn_evp.F90
./cicecore/cicedynB/dynamics/ice_dyn_shared.F90 (for namelist)
./cicecore/cicedynB/general/ice_init.F90 (for namelist)

Namelist (these are the default values, ONLY relevant for revised_evp=.true.):
&dynamics_nml
brlx = 300.0
arlx1i = 1.0/300.0

NOTE: The values for "brlx and arlx1i" are overwritten in ice_dyn_shared for revised_evp=.false.


Have tested identical md5sum's for 24 iterations with GX3 using "revised_evp=.false.".
GNU compiler (Cray XT50) with the following FFLAGS:

FFLAGS := -g -O0 -fsignaling-nans -fno-reciprocal-math -ftrapping-math -fno-associative-math -fno-unsafe-math-optimizations

@mhrib mhrib mentioned this pull request Sep 24, 2018
@apcraig apcraig self-assigned this Sep 27, 2018
@apcraig
Copy link
Contributor

apcraig commented Sep 28, 2018

We need to update the documentation for the new namelist. Does any testing need to change?

@JFLemieux73
Copy link
Contributor

REVP_note.pdf

Till and Mads, I just checked the PR you submitted for the revised EVP. I think it is fine.

However, I don’t think there was a bug in the previous version. These are two valid formulations (see the pdf attached). They should lead to the same solution.

The previous version is more in line with the standard EVP and the REVP in Lemieux2012 (called modified EVP at that time).

I would suggest we use the new one as it is more in line with the recent literature (e.g. Kimmritz 2015, 2016).

I tested both formulations in the gx3 config. For consistency, I set brlx=300 and arlx1i=1/300 in the old formulation. I was hopping to get the same solutions but I didn’t as both formulations did not converge (small errors though). It is my experience that it is not easy to get REVP to converge.

I could test other values of brlx and arlx1i to see if I get convergence.

Let me know if you agree with my conclusions.

jf

@JFLemieux73
Copy link
Contributor

Hi Tony,

I will make a few more tests before we update the documentation. I will let you know.

jf

@apcraig apcraig mentioned this pull request Oct 5, 2018
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 change needs to be accompanied with a new member of our test suite, which demonstrates how different it is from the standard EVP configuration. I'd also like to see the results run through the QC process. Since this is (or at least I consider it a) bug fix, I think we SHOULD merge it, but I'd like to have more information about what the changes look like. Also, JF promised some documentation updates.

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.

We need two things for this PR:

  1. Check the documentation to make sure that it's up to date with these changes. It might be fine, but if the equations changed then let's get them right. We probably do need to reference Kimmritz et al.
  2. Create a test for this, for our test suites. Are the default parameter settings the ones we need to use? Are they resolution dependent?

@JFLemieux73
Copy link
Contributor

Hi everyone,

I made more tests to understand why the revised evp does not converge while others (e.g. Kimmritz et al) claim that it does.

To be more in line with Kimmritz et al I made some tests with kstrength=0 (with Pstar) and I also increased puny from 1e-11 to 2e-9 so that it is consistent with other VP models. Whatever I tried it never converged. In fact the only test that seemed to converge was when I decreased Pstar by two orders of magnitude.

Here is some info about my experiments (for each of these I ran the model for one day with either ndte=25000 or ndte=20000 and subtracted the two thickness fields). dhmax is the maximum (absolute) thickness difference. Note that history_avg=true.


run1
kstrength=1
brlx=300,arlxi=1/brlx

dhmax=8.73e-5 m


run2
kstrength=0
brlx=300,arlxi=1/brlx
Pstar=2.75e4

dhmax=6.12736e-5 m


run3
kstrength=0
brlx=300,arlxi=1/brlx
Pstar=2.75e3

dhmax=9.5367e-7 m


run4
kstrength=0
brlx=300,arlxi=1/brlx
Pstar=2.75e2

dhmax=2e-10 m


run5
kstrength=0
brlx=300,arlxi=1/brlx
Pstar=2.75e4
puny=2e-9 for tinyarea

dhmax=5.54323e-5 m


run6
brlx=500,arlxi=1/brlx
kstrength=0
Pstar=2.75e4
puny=2e-9 for tinyarea

dhmax=1.37568e-4 m


run7
brlx=700,arlxi=1/brlx
kstrength=0
Pstar=2.75e4
puny=2e-9 for tinyarea

dhmax=8.97646e-5 m


run8
brlx=200,arlxi=1/brlx
kstrength=0
Pstar=2.75e4
puny=2e-9 for tinyarea

dhmax= 3.07798e-4 m


run9
brlx=300,arlxi=1/brlx
kstrength=0
Pstar=2.75e4
puny=2e-9 for tinyarea
a) ndte=55000
b) ndte=50000

dhmax= 8.91685e-5 m

my directory for the experiments: /users/dor/armn/jfl/local1/REVP_17oct2018/

@JFLemieux73
Copy link
Contributor

Ok I will take care of comparing against the standard evp and update the documentation.

@JFLemieux73
Copy link
Contributor

Mads,

I wanted to clone again the code for this PR to make some final tests.

The revp branch code does not compile anymore (I think because some of the code in this branch might have changed since the PR).

Could you please add your REVP changes to the current master code and submit a new PR (or resubmit this one) so that I can make these tests?

Thanks

This was referenced Oct 24, 2018
@eclare108213
Copy link
Contributor

superseded by #218 and #219

@mhrib mhrib deleted the rEVP branch November 8, 2023 13:48
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