Skip to content

Fix _get_variable_metadata silently dropping variables#1184

Open
jpdunc23 wants to merge 3 commits into
mainfrom
fix/variable-metadata-silent-drops
Open

Fix _get_variable_metadata silently dropping variables#1184
jpdunc23 wants to merge 3 commits into
mainfrom
fix/variable-metadata-silent-drops

Conversation

@jpdunc23
Copy link
Copy Markdown
Member

XarrayDataset._get_variable_metadata silently omits variables that lack units or long_name attributes, causing variable_metadata to under-report the loaded variable set.

Changes:

  • fme.core.dataset.xarray.XarrayDataset._get_variable_metadata: fall back to empty-string VariableMetadata for variables without units/long_name attrs

  • Tests added

  • If dependencies changed, "deps only" image rebuilt and "latest_deps_only_image.txt" file updated

Resolves #1183

units=ds[name].units,
long_name=ds[name].long_name,
)
else:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code will still silently drop units or long name if the other one is missing, in other words it is all or nothing.

Previously we could disambiguate between units=“” (dimensionless) and missing units, now we can’t. Should we use None in some way to keep this ability, like making this a dict[str, VariableMetadata | None] or by making long name and units optional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, but I think the and meant it was silently dropped if either units or long name were missing? In any case, good idea to keep what is available, will update.

Copy link
Copy Markdown
Contributor

@mcgibbon mcgibbon May 19, 2026

Choose a reason for hiding this comment

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

Yes, these are two separate issues - the first one is pre-existing but should probably get fixed given the scope of this PR. Let's say it's Optional.

The second issue is new in this PR, and is particularly worse if you were to fix the first issue (since long_name="" is kind of a sentinel to say the data is missing, while units="" is a valid value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_get_variable_metadata silently drops variables without units/long_name

2 participants