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

Bounds on derived coords #6190

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

HGWright
Copy link
Contributor

@HGWright HGWright commented Oct 24, 2024

🚀 Pull Request

Description

This PR does the loading agreed in #3678 (from this comment)


Consult Iris pull request check list


Add any of the below labels to trigger actions on this PR:

  • benchmark_this Request that this pull request be benchmarked to check if it introduces performance shifts

Comment on lines 1430 to 1462
if cf_root not in self.cf_group.bounds:
# Check if cf_root has a bounds attribute.
if cf_root in self.cf_group.coordinates:
# Need to generalise this for if it's a dim or aux coord.
bounds_name = getattr(
self.cf_group.coordinates[cf_root], "bounds", None
)
if bounds_name is not None:
form_terms = getattr(
self.cf_group.coordinates[cf_root], "formula_terms"
)
form_terms = form_terms.replace(":", "")
form_terms = form_terms.split(" ")
example_dict = {"a": "A", "b": "B", "ps": "PS", "p0": "P0"}
for cf_vari in formula_terms.values():
for (
cf_roots,
cf_terms,
) in cf_vari.cf_terms_by_root.items():
if cf_terms == cf_term:
if (
cf_roots in self.cf_group.bounds
and cf_roots == bounds_name
):
if cf_terms in form_terms:
to_attach_to = example_dict[cf_terms]
attach_from = cf_vari.cf_name
if (
to_attach_to.lower()
!= attach_from.lower()
):
cf_var.bounds = cf_vari
print(cf_vari.bounds)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if cf_root not in self.cf_group.bounds:
# Check if cf_root has a bounds attribute.
if cf_root in self.cf_group.coordinates:
# Need to generalise this for if it's a dim or aux coord.
bounds_name = getattr(
self.cf_group.coordinates[cf_root], "bounds", None
)
if bounds_name is not None:
form_terms = getattr(
self.cf_group.coordinates[cf_root], "formula_terms"
)
form_terms = form_terms.replace(":", "")
form_terms = form_terms.split(" ")
example_dict = {"a": "A", "b": "B", "ps": "PS", "p0": "P0"}
for cf_vari in formula_terms.values():
for (
cf_roots,
cf_terms,
) in cf_vari.cf_terms_by_root.items():
if cf_terms == cf_term:
if (
cf_roots in self.cf_group.bounds
and cf_roots == bounds_name
):
if cf_terms in form_terms:
to_attach_to = example_dict[cf_terms]
attach_from = cf_vari.cf_name
if (
to_attach_to.lower()
!= attach_from.lower()
):
cf_var.bounds = cf_vari
print(cf_vari.bounds)
bounds_name = None
cf_root_coord = self.cf_group.coordinates.get(cf_root, None)
with contextlib.suppress(AttributeError):
# Copes with cf_root_coord not existing, OR not having
# `bounds` attribute.
bounds_name = _getncattr(cf_root_coord, "bounds")
if bounds_name is not None:
# This will error if more or less than 1 variable is found.
# TODO: does this ever need to be more permissive?
(bounds_var,) = [
f for f in formula_terms.values()
if f.cf_terms_by_root.get(bounds_name) == cf_term
]
self.cf_group[cf_var.cf_name].bounds = bounds_var.cf_name
# TODO: get bounds_var into the appropriate 'cf_group' for
# discovery during helpers.get_cf_bounds_var().
self.cf_group[bounds_var.cf_name] = CFBoundaryVariable(
bounds_var.cf_name, bounds_var.cf_data
)

Copy link
Contributor

Choose a reason for hiding this comment

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

This might also work:

-cf_root_coord = self.cf_group.coordinates.get(cf_root, None)
+cf_root_coord = self.cf_group.coordinates.get(cf_root)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I did not exclude variables that are duplicated between the original formula terms and the bounds formula terms. E.g. ps and p0 in #3678.

This needs doing.

@HGWright HGWright marked this pull request as ready for review October 25, 2024 16:22
@HGWright HGWright changed the title DRAFT Bounds on derived coords Bounds on derived coords Oct 25, 2024
@HGWright
Copy link
Contributor Author

After a long and productive discussion with @trexfeathers and @pp-mo (thanks guys). We have decided that for the upcoming 3.12 release we will put this behind a Future: flag as a different method of loading, this means that we can keep the existing Iris behaviour. We can also avoid making this save to the best version of CF until a later date to avoid this code going stale. (This will be written up and spec'd out in an issue.)

@trexfeathers
Copy link
Contributor

After a long and productive discussion with @trexfeathers and @pp-mo (thanks guys). We have decided that for the upcoming 3.12 release we will put this behind a Future: flag as a different method of loading, this means that we can keep the existing Iris behaviour. We can also avoid making this save to the best version of CF until a later date to avoid this code going stale. (This will be written up and spec'd out in an issue.)

Something else from that discussion, which affects the work in this PR: we need to think about how to handle a case where both approaches to recording bounds have been used, but they produce conflicting information.

@trexfeathers
Copy link
Contributor

My understanding of our goal

... after my most recent conversation with @HGWright

Ref: CF Conventions 7.1.4. Boundaries and Formula Terms

FUTURE OFF 1 FUTURE ON
Primary method 2
Secondary method 3
hybrid_height.nc 4

Footnotes

  1. the current situation

  2. "If a parametric coordinate variable with a formula_terms attribute (section 4.3.2) also has a bounds attribute, its boundary variable must have a formula_terms attribute too. In this case the same terms would appear in both (as specified in Appendix D), since the transformation from the parametric coordinate values to physical space is realized through the same formula. For any term that depends on the vertical dimension, however, the variable names appearing in the formula terms would differ from those found in the formula_terms attribute of the coordinate variable itself because the boundary variables for formula terms are two-dimensional while the formula terms themselves are one-dimensional."

  3. "Whenever a formula_terms attribute is attached to a boundary variable, the formula terms may additionally be identified using a second method: variables appearing in the vertical coordinates' formula_terms may be declared to be coordinate, scalar coordinate or auxiliary coordinate variables, and those coordinates may have bounds attributes that identify their boundary variables. In that case, the bounds attribute of a formula terms variable must be consistent with the formula_terms attribute of the boundary variable. Software digesting legacy datasets (constructed prior to version 1.7 of this standard) may have to rely in some cases on the first method of identifying the formula term variables and in other cases, on the second. Starting from version 1.7, however, the first method will be sufficient."

  4. hybrid_height.nc in iris-sample-data is acknowledged to be incorrect CF, but has been the Iris status quo for years. Iris internals/tests expect this format, and we must assume some users might also depend on this, hence why dropping support for this invalid format must be protected behind a FUTURE flag.

@pp-mo pp-mo linked an issue Feb 20, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Bounds of derived variable are not read correctly
2 participants