Skip to content

Add diagnostic metadata property to constituents object; use existing diagnostic name metadata attribute #695

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

Merged
mkavulich merged 6 commits into from
Jan 15, 2026
Merged

Add diagnostic metadata property to constituents object; use existing diagnostic name metadata attribute #695

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aab4a9e
add diagnostic name property to constituents properties object and us…
peverwhee Nov 18, 2025
9d8fe6a
fix doctest
peverwhee Nov 18, 2025
549e2ac
address review comment
peverwhee Dec 15, 2025
2d189fc
Merge remote-tracking branch 'NCAR/develop' into diagnostic-name
peverwhee Dec 15, 2025
e73a7bc
add additional doctests to new diagnostic name conversion
peverwhee Jan 2, 2026
4601a0e
Merge remote-tracking branch 'NCAR/develop' into diagnostic-name
peverwhee Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions scripts/var_props.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,22 @@ def local_name_to_diag_name(prop_dict, context=None):
Currently, this is just equal to the local name. If no local name
exists in the property dictionary, a truncation of the standard
name is used. (256 characters = max length of NetCDF variable name)
>>> local_name_to_diag_name({'local_name':'foo', 'standard_nam':'cloud_optical_depth'})
>>> local_name_to_diag_name({'local_name':'foo', 'standard_name':'cloud_optical_depth'})
'foo'
>>> local_name_to_diag_name({'standard_name':'cloud_optical_depth_layers_from_0p55mu_to_0p99mu'})
'cloud_optical_depth_layers_from_0p55mu_to_0p99mu'
>>> local_name_to_diag_name({'units':'km'}) #doctest: +ELLIPSIS
Traceback (most recent call last):
...
parse_source.CCPPError: No standard name or local name to convert to diagnostic name
>>> local_name_to_diag_name({'local_name':'', 'standard_name':'cloud_optical_depth'}) #doctest: +ELLIPSIS
Traceback (most recent call last):
...
parse_source.CCPPError: No standard name or local name to convert to diagnostic name
>>> local_name_to_diag_name({'standard_name':''}) #doctest: +ELLIPSIS
Traceback (most recent call last):
...
parse_source.CCPPError: No standard name or local name to convert to diagnostic name
"""
Copy link
Collaborator

@gold2718 gold2718 Dec 30, 2025

Choose a reason for hiding this comment

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

I might add a couple of tests for more complete code coverage:

Suggested change
"""
>>> local_name_to_diag_name({'local_name':'', 'standard_name':'cloud_optical_depth'}) #doctest: +ELLIPSIS
Traceback (most recent call last):
...
parse_source.CCPPError: No standard name or local name to convert to diagnostic name
>>> local_name_to_diag_name({'standard_name':''}) #doctest: +ELLIPSIS
Traceback (most recent call last):
...
parse_source.CCPPError: No standard name or local name to convert to diagnostic name
"""

Copy link
Collaborator Author

@peverwhee peverwhee Jan 2, 2026

Choose a reason for hiding this comment

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

thanks for the extra doctests! added!

diag_name = None
if 'local_name' in prop_dict:
Expand All @@ -155,7 +163,7 @@ def local_name_to_diag_name(prop_dict, context=None):
stdname = prop_dict['standard_name']
if stdname:
maxlen = 256
Copy link
Collaborator

@climbfuji climbfuji Jan 5, 2026

Choose a reason for hiding this comment

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

I wonder if hardcoding this value in this particular place is a good idea. I would be surprised if we didn't have a parameter for the maximum length of a standard name somewhere else already.

Copy link
Collaborator Author

@peverwhee peverwhee Jan 8, 2026

Choose a reason for hiding this comment

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

@climbfuji it looks like we're not enforcing a maximum standard name length anywhere! At least that I can find.

I'm enforcing it here because NetCDF has a maximum length for variable names.

Copy link
Collaborator

@climbfuji climbfuji Jan 8, 2026

Choose a reason for hiding this comment

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

Is that a good opportunity then to define that maxlen parameter in the appropriate place, or do you want to keep it here for now/simplicity?

Copy link
Collaborator Author

@peverwhee peverwhee Jan 8, 2026

Choose a reason for hiding this comment

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

I'm not sure! I don't know if we want a hard limit on the standard name length?

Copy link
Collaborator

@climbfuji climbfuji Jan 9, 2026

Choose a reason for hiding this comment

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

It does sound like a limitation if we say standard names cannot be longer than the maximum string length in netcdf, but it makes total sense to me. I would question the usefulness of standard names longer than 256, maybe even 128 characters.

Copy link
Collaborator Author

@peverwhee peverwhee Jan 9, 2026

Choose a reason for hiding this comment

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

Sounds like something we should discuss at a framework meeting - I'll make an issue.

Copy link
Collaborator

@climbfuji climbfuji Jan 9, 2026

Choose a reason for hiding this comment

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

Thanks! We can come back and clean this up later if needed. Good to merge I think.

diag_name = stdname[:maxlen] if len(stdname) > maxlen else stdname
diag_name = stdname[:maxlen]
# end if
# end if

Expand Down