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 brtemp and cldmask under diag var_struct of `src/core_atmosphere/Registry.xml #1232

Merged

Conversation

jim-p-w
Copy link
Contributor

@jim-p-w jim-p-w commented Sep 16, 2024

Add brtemp and cldmask fields to atmosphere Registry.xml file for MPAS-JEDI.

These fields are required by MPAS-JEDI and are associated with the jedi_da package,
which is active only if config_jedi_da = true.
Without setting config_jedi_da = true this PR should have no impact on
memory usage for stand-alone MPAS-A applications.

@mgduda mgduda self-requested a review September 17, 2024 00:16
@mgduda mgduda changed the base branch from release-v8.2.0 to hotfix-v8.2.2 September 17, 2024 00:17
@mgduda mgduda added the bug fix label Sep 17, 2024
@mgduda
Copy link
Contributor

mgduda commented Sep 17, 2024

@jim-p-w Can you update the PR description and also the commit message to explain why we need to add variables to the MPAS-A Registry that are not used anywhere in the MPAS-A model? Also, it would be worth noting that these variables should have no impact on memory usage for stand-alone MPAS-A applications due to the package that's attached to the variables. As it is, the description is probably not useful for MPAS-A developers, and I think an important function of PR descriptions is to convince maintainers of a repository that code changes should be accepted into the repository.

@jim-p-w jim-p-w force-pushed the atmosphere/add_brtemp_cldmask branch from 43d4bf2 to 8cc229d Compare September 17, 2024 22:23
@jim-p-w
Copy link
Contributor Author

jim-p-w commented Sep 17, 2024

@mgduda The PR description and commit message have been updated.

@mgduda
Copy link
Contributor

mgduda commented Sep 17, 2024

@jim-p-w The commit message begins with "This merge adds brtemp and cldmask under diag var_struct of src/core_atmosphere/Registry.xml (#43)". The commit isn't a merge, and #43 isn't anything related to these changes in the MPAS-Dev/MPAS-Model repository. Also, note that we try to keep the one-line summary of commit messages to 80 characters or less.

I would also suggest removing the bit about madwrf and SACA from the PR description, removing the "DESCRIPTION OF CHANGES:" and "LIST OF MODIFIED FILES:" lines, and also removing the part about cherry-picking.

@jim-p-w
Copy link
Contributor Author

jim-p-w commented Sep 17, 2024

How about
For the commit message:

Add brtemp and cldmask under diag var_struct of src/core_atmosphere/Registry.xml

These fields are used by MPAS-JEDI and are associated with the `jedi_da` package,
which is active only if `config_jedi_da = true`.
Without setting `config_jedi_da = true` this commit should have no impact on
memory usage for stand-alone MPAS-A applications.

For the PR message use the same verbiage, but change commit to PR

@mgduda
Copy link
Contributor

mgduda commented Sep 18, 2024

How about For the commit message:

Add brtemp and cldmask under diag var_struct of src/core_atmosphere/Registry.xml

These fields are used by MPAS-JEDI and are associated with the `jedi_da` package,
which is active only if `config_jedi_da = true`.
Without setting `config_jedi_da = true` this commit should have no impact on
memory usage for stand-alone MPAS-A applications.

For the PR message use the same verbiage, but change commit to PR

I think it would be nice if there were a way to include "MPAS-JEDI" in the one-line summary, which we'd see with, e.g., git log --oneline. How about something like "Add brtemp and cldmask fields to atmosphere Registry.xml file for MPAS-JEDI" for the first line?

Also, perhaps rather than "used by MPAS-JEDI", would it be fair to say "required by MPAS-JEDI"?

…S-JEDI

These fields are required by MPAS-JEDI and are associated with the `jedi_da` package,
which is active only if `config_jedi_da = true`.
Without setting `config_jedi_da = true` this commit should have no impact on
memory usage for stand-alone MPAS-A applications.
@jim-p-w jim-p-w force-pushed the atmosphere/add_brtemp_cldmask branch from 8cc229d to 9d312db Compare September 18, 2024 15:59
@jim-p-w
Copy link
Contributor Author

jim-p-w commented Sep 18, 2024

@mgduda I've done another update to the PR and commit verbiage .

Copy link
Contributor

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

I can confirm that I see no increase in memory usage when running stand-alone MPAS-A with config_jedi_da = false.

@mgduda mgduda merged commit 35bd8b8 into MPAS-Dev:hotfix-v8.2.2 Sep 19, 2024
mgduda added a commit that referenced this pull request Sep 20, 2024
This merge addresses several issues in the MPAS-Atmosphere model and in the MPAS
infrastructure. Specific changes in this merge include:

* Fix to a portability issue in the MPAS registry 'parse' tool, which caused
  files in the src/core_<CORE>/inc directory to not be generated correctly at
  build time on some systems (PR #1229).

* Addition of two fields, brtemp and cldmask, that are needed by MPAS-JEDI.
  Although not needed by stand-alone MPAS-Atmosphere, these fields are
  associated with the 'jedi_da' package and therefore have no effect when
  MPAS-Atmosphere is run without setting config_jedi_da = true (PR #1232).

* Removal of a redundant query of the nCellsSolve dimension in the
  physics_run_init routine. The extra query had no impact on results, and its
  removal can be considered clean-up (PR #1236).
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.

3 participants