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

Remove Tracer CPPs, improve scripts test performance, and update documentation #196

Merged
merged 10 commits into from Oct 10, 2018

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Oct 4, 2018

Moved Tracer CPPs and numin/numax to namelist, improve scripts, update documentation for dynamics allocation.

  • Developer(s): tcraig

  • Are the code changes bit for bit, different at roundoff level, or more substantial? bit-for-bit except a few cray results

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

  • Is the documentation being updated with this PR? (Y/N) Y

  • Other Relevant Details:

Move tracer cpps to namelist
Move numin/numax to namelist
Update scripts performance for testing by supporting executable reuse and reducing cost of running cice.setup by adding --nomodules option to env_mach_specific call.
Update documentation to reflect changes from static to dynamic memory allocation

See hash 627b105, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks

See https://apcraig-cice.readthedocs.io/en/ciceteste/index.html

@duvivier
Copy link
Contributor

duvivier commented Oct 4, 2018

@apcraig
The documentation looks good, but I have one request about dg_dynamics.rst:
You have:
"CICE5 was implemented using mainly static arrays and required several CPPs to be set to define grid size, blocks sizes, tracer numbers, and so forth. With CICE6, arrays are now dynamically allocated and those parameters are now namelist settings. The following CPPs are no longer used in CICE6,..."

Suggested change:
"CICE versions 5 and earlier were implemented using mainly static arrays and required several CPPs to be set to define grid size, blocks sizes, tracer numbers, and so forth. With CICE versions 6 and later, arrays are dynamically allocated and those parameters are namelist settings (see :ref:tabnamelist). "

I don't think you need the old list of CPPs personally, but I don't feel too strongly about that. Do you want to list all the new namelist options here since they span the setup_nml, domain_nml, and tracer_nml sections of the namelist? I don't feel strongly one way or the other about this either, but just something we could do to emphasize what users need to change.

@apcraig apcraig added this to To do in CICE v6 release via automation Oct 5, 2018
CICE v6 release automation moved this from To do to In progress Oct 5, 2018
eclare108213
eclare108213 previously approved these changes 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.

Do you understand why the cray regression tests are failing?
I'm fine with this PR, but the conflicts will have to be addressed before it's merged.

@apcraig
Copy link
Contributor Author

apcraig commented Oct 8, 2018

I have rebased the branch and updated some documentation. I ran some quick_suite on conrad on 4 compilers after the rebase and things seems to be still the same.

@duvivier, i have updated the documentation at your suggestion. I think it's better, thanks. I think in a year or two, we should remove references to the cpps from CICE5 and before, but they have been such an important part of using CICE up to now, I think it's helpful to note that they have changed in CICE6.

@eclare108213, I looked further into the cray non-bit-for-bit changes. I figured the issue was either different "fill" (over land) values due to how the initialization of the allocation dynamics vs statically was done. Or it was due to changes in compilation optimization. I ran some more tests and now believe it is compiler optimization. The non bit-for-bit results are only for a small subset of cray compiler tests. I have confirmed that in those cases, the results are changing in the science, not just in the land values. I am going to try a couple more runs with reduced compiler optimization to further confirm.

duvivier
duvivier previously approved these changes Oct 8, 2018
@apcraig apcraig self-assigned this Oct 8, 2018
@apcraig
Copy link
Contributor Author

apcraig commented Oct 8, 2018

Just another data point for the non bit-for-bit issue for some Cray tests. I took one of the failing tests, conrad_cray_restart_gx3_8x2_alt02, and ran it with the current master and this branch with compiler optimization O0, O1, and O2. O2 is our current default. O0 and O1 are both bit-for-bit. O2 is not. So this is a compiler optimization issue for a small set of tests with the cray compiler.

@apcraig
Copy link
Contributor Author

apcraig commented Oct 9, 2018

@duvivier @eclare108213, @dabail10

Had to quickly resolve another documentation conflict with PR#201, just pulled. I think the reviews get reset whenever there is a new commit. I could change that, but could someone just give a quick review then I'll merge the PR after the current travis check is complete. thanks.

duvivier
duvivier previously approved these changes Oct 9, 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 looks good to me, except for the location of these parameters in ice_in:
ncat = 5
nilyr = 7
nslyr = 1
nblyr = 7

Those are vertical grid sizes (well, sort of, for ncat), and so I think they should be in grid_nml instead of tracer_nml. The tracers certainly depend on them, but they are fundamentally for discretizing equations in the physics (well, nblyr is more about tracers, but for consistency I'm including it here too).

@apcraig
Copy link
Contributor Author

apcraig commented Oct 9, 2018

thanks @eclare108213, i'll fix the namelist.

@apcraig
Copy link
Contributor Author

apcraig commented Oct 9, 2018

@eclare108213 , OK moved the 5 namelist from tracer to grid, tested quick_suite on 4 compilers on conrad, all seems OK.

@eclare108213 eclare108213 merged commit fe8c815 into CICE-Consortium:master Oct 10, 2018
CICE v6 release automation moved this from In progress to Done Oct 10, 2018
@apcraig apcraig deleted the cicetestE branch August 17, 2022 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants