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

Advection test - slotted cylinder #254

Merged
merged 13 commits into from
Nov 29, 2018

Conversation

phil-blain
Copy link
Member

@phil-blain phil-blain commented Nov 26, 2018

Implemented the slotted cylinder advection test.

  • Developer(s): Philippe Blain

  • Please suggest code Pull Request reviewers in the column at right. @eclare108213, others?

  • Are the code changes bit for bit, different at roundoff level, or more substantial? should be BFB

  • Please include the link to test results or paste the summary block from the bottom of the testing output below.
    47 of 115 tests PASSED
    0 of 115 tests FAILED
    33 of 115 tests PENDING
    -> I still have a porting issue, the tests build fine but don't seem to run correctly (the log show they receive SIGTERM), I will try to fix this and rerun them.

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

  • Is the documentation being updated with this PR? (Y/N) Yes
    If not, does the documentation need to be updated separately at a later time? (Y/N)
    Test description
    Namelist changes (ice_data_type)
    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:

  • I followed [1] for the geometry of the slotted cylinder. This is the paper cited in [2] for the slotted cylinder test, [2] being the paper which was referenced by Elizabeth in configure rectangular "box" case for advection tests #172.
  • The slot is normal to the velocity field, as in [1]. Some other papers place it tangentially, this can easily be modified (or added as a second option)
  • I chose to directly impose the ice velocity. This was the consensus here between Fred, JF and me.
  • The test has been tested only on the 80x80 grid, not yet on the 128x128 grid.
  • I'm not sure if more options would be appropriate in set_nml.boxslotcyl. Please advise.
  • I did not create a test in the configuration/scripts/tests directory, I don't know if that is what was understood by advection "test". I will update the PR if need be.
  • Currently the test is configured with a speed such that the cylinder makes one revolution in 12 days, with a time step of one hour and a mesh size of dxrect=dyrect=10 km. This gives a Courant number of 0.86.

[1] S. T. Zalesak, “Fully multidimensional flux-corrected transport algorithms for fluids,” Journal of Computational Physics, vol. 31, no. 3, pp. 335–362, Jun. 1979.
[2] W. H. Lipscomb and T. D. Ringler, “An Incremental Remapping Transport Scheme on a Spherical Geodesic Grid,” Monthly Weather Review, vol. 133, no. 8, pp. 2335–2350, Aug. 2005.

Documentation said they were in meters but in the code they are in centimeters
Also :  minor correction in the documentation of the box2001 test, added link to box2001 doc from Table of namelist options
@phil-blain
Copy link
Member Author

I just added documentation.

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 is a good start. There's one major change that I'd like to see: revert the changes in ice_dyn_shared.F90. In particular, the contents of subroutine boxslotcyl_data could be moved into the subroutine (of the same name) in ice_init.F90. Since you're not using the dynamics calculation and imposing the ice velocity directly, you can use kdyn=-1 as a conditional if necessary to subvert ice velocity initialization in the main code.
The new namelist ice_data_type makes sense to me, thanks for implementing it. Related to where to move the code now in ice_dyn_shared.F90, I would like to allow this configuration to be flexible for use with other tests besides simple advection, which means keeping the ice initial condition separate from the forcing. I'm not sure how easy this is to do, or if it makes sense in this PR (we can always change the code later) -- we can discuss this offline if it's not obvious what to (not) do.
Looks like you need at return at the end of the set_nml.boxslotcyl file.

@eclare108213 eclare108213 self-assigned this Nov 27, 2018
@eclare108213
Copy link
Contributor

I have made a few small changes to this PR. I'm unable to do a complete set of tests because our computing allocation here at LANL disappeared this week and will take a few hours, at least, to propagate back to the machines. In the meantime, I ran the basic test on 1 processor, which appears to work well. If someone can run a base_suite with regression comparison and post BFB results, I think we can merge this. Otherwise I'll try again tomorrow. (Philippe and JF are out of the office for the next couple of days)

@apcraig
Copy link
Contributor

apcraig commented Nov 28, 2018

I will run a test suite on conrad or gordon this evening.

@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2018

A full test suite was run on conrad with 4 compilers, results are here https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks hash 6258ead. I think things are OK, maybe. This does not have the eap fix in it so boxadv debug test is still failing. That is fixed by the #255. Otherwise, things are bit-for-bit except for the box test results. In fact, the only box test to remain bit-for-bit is restart_gbox80_1x1_box2001. All the others produce different results. Are the box test results supposed to be bit-for-bit or not? Looking at a random box test (restart_gbox128_4x2), the results are quite different, much bigger than roundoff. Below are total ice area from the log files, current branch,

total ice area  (km^2) =    1.42835402008975577E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42836416161911357E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42837422139390819E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42836884374719877E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42836099961879477E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42837130107695125E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42838114766296446E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42837471596985683E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42836652785296235E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42837594123122375E+07   0.00000000000000000E+00

and baseline,

total ice area  (km^2) =    1.42821552746070605E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42822801323475279E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42823902395919282E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42825730598447546E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42826545029119458E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42827462930456325E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42828301582181640E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42828614236087911E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42828443174161520E+07   0.00000000000000000E+00
total ice area  (km^2) =    1.42829369727073293E+07   0.00000000000000000E+00

almost like the initial conditions are different or something. Is this OK/expected?

@eclare108213
Copy link
Contributor

The results should be BFB. Philippe added a new namelist flag, ice_data_type, and the logic might have changed how the other box tests are behaving -- maybe it needs to be set for those tests too, in addition to/instead of atm_data_type? I'll look.

@phil-blain
Copy link
Member Author

Elizabeth, you were right, I forgot to add ice_data_type to the box grid options files. It should be BFB now.

@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2018

I've submitted a full test suite again on conrad. Will let you know (probably in the morning).

Also, there is now a conflict in one of the documentation files. That will need to be fixed before we can proceed. I'm okay fixing it via the github "resolve conflicts" button if that works for others. Or maybe someone can resolve it in their sandbox and push. If anyone needs me to help with that, let me know.

@phil-blain
Copy link
Member Author

I tried to resolve the conflict.

@eclare108213
Copy link
Contributor

I think I fixed the conflict in the web browser. @duvivier please check ug_case_settings.rst, thx.

We ought to fix this in the code so that the units are m...  cm is left over from POP grid files.
@apcraig
Copy link
Contributor

apcraig commented Nov 29, 2018

The conrad tests are now passing as expected. See hash c3478b3 at https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks. The one failure is related to the eap fix which is not on this branch. I don't see any questions related to testing at this point. I am fine to merge if others agree on other aspects (documentation, validated of slotted cylinder results, etc).

@eclare108213 eclare108213 merged commit 591ea07 into CICE-Consortium:master Nov 29, 2018
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.

3 participants