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

add MACHINFO and ENVINFO, remove thunder and loft #323

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Jun 22, 2020

PR checklist

  • Short (1 sentence) summary of your PR:
    add MACHINFO and ENVINFO, remove thunder and loft

  • 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 a quick suite on cheyenne on 3 compilers to confirm the new information is available in the results. No code was modified, just scripts. https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#b0b7162f8759f9e0a16dcca6f52a5b9ea268be7d

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

  • Add MACHINFO and ENVINFO to bring in line with recent CICE changes

  • Rename ICE_COMPILER to ICE_ENVNAME, ICE_MACHINE_COMPILER to ICE_MACHINE_MACHNAME, ICE_MACHINE_COMPILER to ICE_MACHINE_ENVNAME for better consistency with icepack.setup arguments.

  • rename some icepack.setup variables for better consistency

  • Remove machines thunder and loft

  • Fix error in env.conda_macos. it was loading the cice env, changed this to the icepack env which is a subset or the cice env (which is why it was working).

  • Add -nomodules option to several env files to improve script performance.

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.

envname is a little confusing for me, especially since it previously was used for something different. To clarify, it would help to say in more places that it's referring to compilers (but I assume it's more general than that, since you're changing the definition). I've made a couple of recommendations below. Otherwise this is all fine with me.

icepack.setup Outdated
@@ -91,7 +91,7 @@ DESCRIPTION
--docintfc : update the public interface documentation
--case, -c : case, case directory/name (not with --test or --suite)
--mach, -m : machine, machine name (required)
--env, -e : compiler(s), comma separated (default = "intel")
--env, -e : envname(s), comma separated (default = "intel")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't recognize that envname means compiler, although it helps that you indicate that the default is intel. Maybe 'environment/compiler(s)' ?

Copy link
Member

Choose a reason for hiding this comment

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

maybe "compilation environment" ?

@@ -27,7 +27,7 @@ can be modified as needed.
"ICE_CASENAME", "string", "case name", "set by icepack.setup"
"ICE_SANDBOX", "string", "sandbox directory", "set by icepack.setup"
"ICE_MACHINE", "string", "machine name", "set by icepack.setup"
"ICE_COMPILER", "string", "environment name", "set by icepack.setup"
"ICE_ENVNAME", "string", "environment name", "set by icepack.setup"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add clarification here:
environment name (compiler)

@apcraig
Copy link
Contributor Author

apcraig commented Jun 23, 2020

Good ideas @eclare108213 and @phil-blain. I have updated the documentation. Let me know if that seems better.

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, thanks

@apcraig apcraig merged commit 0a37e99 into CICE-Consortium:master Jun 23, 2020
lettie-roach pushed a commit to lettie-roach/Icepack that referenced this pull request Oct 18, 2022
* Fix local solar time

* Add line for going past 24

* New LST calculation

Addresses #3.
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.

None yet

3 participants