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

Allow alternative mangle for LONG_NAME (R21C) #2860

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

mathomp4
Copy link
Member

@mathomp4 mathomp4 commented Jun 7, 2024

Types of change(s)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist

  • Tested this change with a run of GEOSgcm
  • Ran the Unit Tests (make tests)

Based on user request, this PR adds the capability to supply an alternative prefix for mangling LONG_NAME instead of the default comp_name. In spec files, * can be added to the SHORT_NAME and LONG_NAME, and this PR allows the alternative prefix.

In order to use this in GEOS, you'll need to update your CMakeLists.txt with the new option. For example, this:

mapl_acg (${this}   CA2G_StateSpecs.rc
          IMPORT_SPECS EXPORT_SPECS INTERNAL_SPECS
          GET_POINTERS DECLARE_POINTERS
          LONG_NAME_PREFIX "GCsuffix")

would tell the ACG to use the variable GCsuffix for the long_name.

NOTE1: This variable must be in the GC where this is being used. This works for CA because of this code:

    if (comp_name(1:5) == 'CA.oc') then
       GCsuffix = 'OC'
    else if (comp_name(1:5) == 'CA.bc') then
       GCsuffix = 'BC'
    else if (comp_name(1:5) == 'CA.br') then
       GCsuffix = 'BR'
    end if

which fills in GCsuffix for each instance.

NOTE2: This is 0-diff in the sense that the state of the model does not change if you enable this option. But it will change both restarts and history output due to the change in metadata.

Related Issue

#2847

@mathomp4
Copy link
Member Author

mathomp4 commented Jun 7, 2024

@acollow Can you try this branch out? You'll want to change the GOCART as said above.

Once you do, please make sure history looks like what you want. Especially look at:

@mathomp4 , Note that M21C will also need the MAPL patch that corrects the "001" bug.

Please see if it is doing what you want or not. If not, tell us what you want and then @atrayano can get the appropriate fix in

Copy link

github-actions bot commented Jun 7, 2024

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled, github_actions. Please add one so that the PR can be merged.

@mathomp4 mathomp4 added the 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. label Jun 7, 2024
Copy link

github-actions bot commented Jun 7, 2024

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled, github_actions. Please add one so that the PR can be merged.

@acollow
Copy link

acollow commented Jun 10, 2024

@mathomp4, @darianboggs Is it possible to clarify which CMakeLists.txt to add this too?

@mathomp4
Copy link
Member Author

@mathomp4, @darianboggs Is it possible to clarify which CMakeLists.txt to add this too?

@acollow It's the Cmake for whatever component needs this. So in this case, CA. So edit .../CA2G_GridComp/CMakeLists.txt and change:

mapl_acg (${this}   CA2G_StateSpecs.rc 
          IMPORT_SPECS EXPORT_SPECS INTERNAL_SPECS 
          GET_POINTERS DECLARE_POINTERS)

to:

mapl_acg (${this}   CA2G_StateSpecs.rc
          IMPORT_SPECS EXPORT_SPECS INTERNAL_SPECS
          GET_POINTERS DECLARE_POINTERS
          LONG_NAME_PREFIX "GCsuffix")

where that last line is added.

@acollow
Copy link

acollow commented Jun 10, 2024

I am confirming that I tested the branch and the carbon long names are now being written in an ideal manner. Thank you! The branch however still has the bug for the dust and sea salt bins. I copied below the output from ncdump on a file I generated showing what should be SS001 and SS002.

float SS(time, lev, nf, Ydim, Xdim) ;
	SS:_FillValue = 1.e+15f ;
	SS:add_offset = 0.f ;
	SS:coordinates = "lons lats" ;
	SS:fmissing_value = 1.e+15f ;
	SS:grid_mapping = "cubed_sphere" ;
	SS:long_name = "Sea Salt Mixing Ratio (bin 001)" ;
	SS:missing_value = 1.e+15f ;
	SS:regrid_method = "identity" ;
	SS:scale_factor = 1.f ;
	SS:standard_name = "Sea Salt Mixing Ratio (bin 001)" ;
	SS:units = "kg kg-1" ;
	SS:valid_range = -1.e+15f, 1.e+15f ;
	SS:vmax = 1.e+15f ;
	SS:vmin = -1.e+15f ;
float SS002(time, lev, nf, Ydim, Xdim) ;
	SS002:_FillValue = 1.e+15f ;
	SS002:add_offset = 0.f ;
	SS002:coordinates = "lons lats" ;
	SS002:fmissing_value = 1.e+15f ;
	SS002:grid_mapping = "cubed_sphere" ;
	SS002:long_name = "Sea Salt Mixing Ratio (bin 002)" ;
	SS002:missing_value = 1.e+15f ;
	SS002:regrid_method = "identity" ;
	SS002:scale_factor = 1.f ;
	SS002:standard_name = "Sea Salt Mixing Ratio (bin 002)" ;
	SS002:units = "kg kg-1" ;
	SS002:valid_range = -1.e+15f, 1.e+15f ;
	SS002:vmax = 1.e+15f ;
	SS002:vmin = -1.e+15f ;

@atrayano
Copy link
Contributor

@acollow Allie, could you please point me to your History.rc file. I just want to make sure I fully understand the requirements. My dilemma is when the user provides a single alias, do we always append 00n to the alias, or we take the alias value and leave it as is. One quick workaround (but not a fix), is to always provide an alias for the field as in the following example 'DUEM' , 'DU' , 'DUEM001;DUEM002;DUEM003;DUEM004;DUEM005'

@acollow
Copy link

acollow commented Jun 10, 2024

@atrayano, you're right! Neither my HISTORY.rc nor the one for M21C have the aliases. I just ran a quick test and that solved it. Thanks!

@atrayano
Copy link
Contributor

@acollow I am happy now! Thanks! The rules for split names are a bit confusing. If you do not have alias(es), then MAPL tries to keep the field name in the output the same. But in case of multiple bins, having two different fields with the same name is not possible, and to disambiguate, we append 00n, but starting at 2 to keep the original name the same in case of a single bin. On the other hand, if you provide enough split-field-aliases, we keep them as is.

@mathomp4 mathomp4 marked this pull request as ready for review June 10, 2024 18:45
@mathomp4 mathomp4 requested review from a team as code owners June 10, 2024 18:45
@mathomp4
Copy link
Member Author

Since @acollow says this works for her I will undraft. Once all is good, I can work with @sdrabenh to pull in and make a new R21C release

@mathomp4 mathomp4 requested a review from elakkraoui June 13, 2024 13:26
Copy link

@elakkraoui elakkraoui left a comment

Choose a reason for hiding this comment

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

@mathomp4 , @acollow, I approved this PR but unless I missed something, I see that Allie still needs to submit another PR with the Cmake update for the change to take effect.

@mathomp4
Copy link
Member Author

@mathomp4 , @acollow, I approved this PR but unless I missed something, I see that Allie still needs to submit another PR with the Cmake update for the change to take effect.

@elakkraoui They can be taken separately, so we can merge this in at any time (same for #2852).

This just adds the ability for @acollow to make a change in GOCART to enable it.

@elakkraoui
Copy link

@mathomp4 , @acollow, I approved this PR but unless I missed something, I see that Allie still needs to submit another PR with the Cmake update for the change to take effect.

@elakkraoui They can be taken separately, so we can merge this in at any time (same for #2852).

This just adds the ability for @acollow to make a change in GOCART to enable it.

Sounds good.

@acollow
Copy link

acollow commented Jun 21, 2024

Totally spaced that I needed to add the PR to GOCART! Sorry about that! I just added it - GEOS-ESM/GOCART#278

@tclune tclune merged commit 9ff79d7 into R21C Jun 21, 2024
13 checks passed
@mathomp4 mathomp4 deleted the feature/wdboggs/longname_mangle-R21C branch June 21, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants