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.

Sign up for GitHub

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

Jump to bottom

Update CPPs and Documentation #329

Merged
merged 9 commits into from
Jul 31, 2020
Merged

Update CPPs and Documentation #329

merged 9 commits into from
Jul 31, 2020

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Jul 23, 2020

PR checklist

  • Short (1 sentence) summary of your PR:
    Update CPPs and Documentation
  • 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.
    Ran quick suite on 4 compilers on gordon. Should not affect standalone Icepack. https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#8efeb5970607894ee4286684680f4acb57614ae8
    Also tested in CICE with Update CPP implementation CICE#490
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • 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:

eclare108213
eclare108213 approved these changes Jul 24, 2020
View reviewed changes
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.

It seems like the NOFRAC cpp ought to be in namelist, and be changed to something specific to isotopes, if indeed it is specific to isotopes... but that doesn't have to be done in this PR.

doc/source/user_guide/ug_running.rst

For standalone Icepack, The CPPs are defined by the `CPPDEFS` variable in the Icepack
Makefile. They are defined
by passing the -D[CPP] to the C and Fortran compilers (ie. -DNO_I8) and this
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of 'ie.' please use 'e.g.'
I don't know the Latin, but I think i.e. means "that is," and e.g. means "for example,"

@apcraig
Copy link
Contributor Author

apcraig commented Jul 27, 2020

I agree the NOFRAC should be moved to namelist. I was thinking that might take some coordination with CICE and think it will, but I believe the Icepack upgrade can be done first. I'll update this PR with that change. Good idea.

@apcraig
Copy link
Contributor Author

apcraig commented Jul 27, 2020

The NOFRAC cpp has been removed. The quick suite was run with 4 compilers on gordon and all results are bit-for-bit.

dabail10
dabail10 approved these changes Jul 27, 2020
View reviewed changes
columnphysics/icepack_kinds.F90
r16_kind = selected_real_kind(33,4931)
#endif
Copy link
Contributor

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, but I have a dumb question. I'm not familiar with how selected_real_kind works, but it seems to me there might be a machine and compiler dependence here? Do we have to update this every couple of years? Sorry if I missed this in an earlier discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NO_I8 and NO_R16 are definitely compiler dependent which means they may be required on certain machines. And this could certainly change. For instance, NO_R16 is defined for a few machine/compilers now. It's possible those are incorrect. To be honest, I'm not sure how those settings were decided. I suspect it's partly historical and they have not been reviewed for a while. The main point of these CPPs is that they provide users with a flag to turn off these features if their compiler doesn't support them. As a general rule, they should only be used when needed. And we probably should review the current settings in the current machines to check whether we really do need them where they are currently set.

phil-blain
phil-blain reviewed Jul 31, 2020
View reviewed changes
columnphysics/icepack_kinds.F90
@@ -17,10 +18,18 @@ module icepack_kinds
char_len_long = 256, &
log_kind = kind(.true.), &
int_kind = selected_int_kind(6), &
#if defined (NO_I8)
Copy link
Member

Choose a reason for hiding this comment

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

Minor point, but why #if defined and not #ifdef ? I think they do the same thing but maybe we should stick to one or the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CICE, there is a mixture of #ifdef and #if defined. The NO_I8 in CICE has #if defined, so I made the icepack format match that. I could change it here, but then would probably want to change it in CICE as well. If we want to take this step, I'm happy to do it. I'm personally not that concerned about mixing the styles, although agree a single approach is nicer (but then requires diligence to maintain it).

doc/source/user_guide/ug_case_settings.rst Outdated Show resolved Hide resolved
doc/source/user_guide/ug_running.rst Outdated Show resolved Hide resolved
doc/source/user_guide/ug_running.rst Outdated Show resolved Hide resolved
doc/source/user_guide/ug_running.rst
support certain coding features to be excluded or included during the compile. They
exist in part to support the CICE model and other applications that use Icepack.

For standalone Icepack, The CPPs are defined by the `CPPDEFS` variable in the Icepack
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For standalone Icepack, The CPPs are defined by the `CPPDEFS` variable in the Icepack
For standalone Icepack, The CPPs are defined by the ``CPPDEFS`` variable in the Icepack

doc/source/user_guide/ug_running.rst Outdated Show resolved Hide resolved
columnphysics/icepack_isotope.F90
Comment on lines -485 to -487
#ifdef NOFRAC
wiso_alpi = c1
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This is the only instance of "NOFRAC"... was this added for compiling CICE as part of another model ? (@eclare108213 you added that in 1ae0446 (Isotopes for Icepack (#307), 2020-04-06))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to @dabail10, this was added for debugging the isotopes and is not needed at this time.

apcraig and others added 4 commits July 31, 2020 08:56
@apcraig @phil-blain
Update doc/source/user_guide/ug_running.rst
e5a2356 
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
@apcraig @phil-blain
Update doc/source/user_guide/ug_case_settings.rst
a39f619 
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
@apcraig @phil-blain
Update doc/source/user_guide/ug_running.rst
9f1244d 
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
@apcraig @phil-blain
Update doc/source/user_guide/ug_running.rst
8dc0ec6 
Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com>
phil-blain
phil-blain approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member

@phil-blain phil-blain 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 also.

Just a note (you might already know); you can check the "add suggestion to batch" when I make suggestions so that there is only one additional commit with all my suggestions squashed. But since we squash-merge PRs it does not really matter as the individual commits are discarded on master.

@apcraig apcraig merged commit 4c42a82 into CICE-Consortium:master Jul 31, 2020
@apcraig apcraig mentioned this pull request Jul 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclare108213 eclare108213 eclare108213 approved these changes

@dabail10 dabail10 dabail10 approved these changes

@phil-blain phil-blain phil-blain approved these changes

Assignees
No one assigned
Labels
Build Documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apcraig @eclare108213 @dabail10 @phil-blain