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

Fix issue #293 NUOPC_CompAttributeIngest failure #316

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

danrosen25
Copy link
Member

@danrosen25 danrosen25 commented Oct 31, 2024

Fix #293
Append a token lists into single spaced string

@danrosen25 danrosen25 self-assigned this Oct 31, 2024
@danrosen25 danrosen25 added the bug Something isn't working label Oct 31, 2024
Copy link
Member

@theurich theurich 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. The way I interpret the new code, the use ends up with a single string value for both cases:

<COMP>_attributes::
      restart_fh = 0.05,0.25,0.5,3,6
 ::

and

<COMP>_attributes::
      restart_fh = 0.05, 0.25, 0.5, 3, 6
 ::

I think that is consistent behavior for how the non-HConfig Attribute handling is working string based.

@danrosen25
Copy link
Member Author

@theurich It does but note that

<COMP>_attributes::
      restart_fh = 0.05, 0.25, 0.5, 3, 6
 ::

and

<COMP>_attributes::
      restart_fh = 0.05,     0.25,          0.5, 3,  6
 ::

both return 0.05, 0.25, 0.5, 3, 6

So although it works with whitespace now

<COMP>_attributes::
      my_line = I  am  a  sentence  with  double  spacing
 ::

returns I am a sentence with double spacing
and that's not what a user would expect

I had to make a change because the NUOPC Comp UTest was failing.

@danrosen25
Copy link
Member Author

@tclune @NickSzapiro-NOAA
Hi,
I'm looking for user feedback for this change to NUOPC. It's become standard practice to read ESMF Config tables into a FreeFormat then ingest the FreeFormat into component attributes. We discovered a bug in which ESMF Config table rows that include spaces were being completely ignored restart_fh = 0.05, 0.25, 0.5, 3, 6 during this process. This happens because reading config into a FreeFormat treats spaces as column separators, which is how we support tables. When ingesting a FreeFormat into an attribute NUOPC does not know how to handle multiple attribute values for a single key.

This fix will concatenate all of the items with one space between each item, which is then added to the component's attributes. It's not without issues because it will trim all excess white space. See code for specific technical details.

I did play around with ingesting multiple values as a valueList but there was little benefit.

@tclune
Copy link
Collaborator

tclune commented Nov 5, 2024

@danrosen25 I doubt this will affect GEOS, but if you want us to test a branch with GEOS, we'll be happy to oblige.

Wish I could say we are entirely HConfig at this point, but production systems will be mostly Config for at least another year, not to mention legacy variants.

@tclune
Copy link
Collaborator

tclune commented Nov 5, 2024

Sorry - was assuming this was a change in the underlying ESMF_Config. GEOS itself does not use any NUOPC code, so this should only affect UFS.

@NickSzapiro-NOAA
Copy link

Hi @danrosen25. Sorry for the slow reply as I was on PTO.

I'm not aware of any need to maintain white space. It's certainly better to be able to read the attribute (as string then parse)

In the current NUOPC_refdoc, there are two:

NUOPC_CompAttributeGet - string list
NUOPC_CompAttributeGet - integer list

If these are not implemented/supported, maybe this could be noted (only for the ESMF config format?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NUOPC_CompAttributeIngest fails (quietly) if line has spaces
4 participants