Skip to content

Restore lost fixes - #67

Open
sol1105 wants to merge 4 commits into
developfrom
restore_lost_fixes
Open

Restore lost fixes#67
sol1105 wants to merge 4 commits into
developfrom
restore_lost_fixes

Conversation

@sol1105

@sol1105 sol1105 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

This PR deals with the merged fixes that were overwritten as detailed in issue #66
I found that three fixes have disappeared (but a second pair of eyes conforming this would not hurt):

  • 8bd1699 (include subdaily frequencies in AVERAGE_CORRECTION_FREQ for check TIME001)
  • bfb2b9f (flag valued geophysical variables)
  • b7cc340 (geophysical variable detection)

I also added the dec frequency to the AVERAGE_CORRECTION_FREQs that are evaluated at the midpoint of the time interval, if not time: point is specified as cell_methods.

closes #66

JanStreffing and others added 4 commits August 5, 2026 07:54
…cal variables

CMIP7 region-selector fx files (basin, siline, similar) carry their
data variable as a CF flag-valued integer (flag_values + flag_meanings
mapping integer codes to region/section names). compliance-checker's
is_geophysical excludes any variable with flag_meanings from the
geophysical-variable set as a heuristic for status flags. The exclusion
is right for QC flags but wrong for these region selectors, whose
standard_name is "region" not "status_flag", and which ARE the file's
data variable by CMIP7 design.

The existing variable_id disambiguation in _get_geo_var only fires when
CF returned multiple candidates. When CF returns zero (the basin/siline
case) the function bails with "No geophysical variable detected in the
file." at HIGH severity, which blocks ESGF publication.

Extend the variable_id fallback to the zero-candidate branch. If CF
detection finds nothing and the global variable_id attribute names an
existing variable, accept that variable as the file's geophysical
variable. The strict CF heuristic stays as the primary path; the
fallback only kicks in when both (a) CF found zero candidates AND
(b) the CMIP7-canonical variable_id attribute is present.
CMIP7 (and CMIP6 archive practice) ships sub-daily tavg files with
midpointed time stamps. _is_instantaneous already handles tpt rules
via cell_methods, so adding the bare freq names only changes behaviour
for tavg sub-daily, in the direction the spec implies.

Closes #51.
Closes #59.

CMIP7 region-selector variables (basin, siline, similar) are CF flag-
valued integers. The default geophysical_variable.toml rules describe
a canonical continuous float field (data_type = float, _FillValue =
1e20, missing_value = 1e20), which cannot express the flag case and
cannot be conditionalised in TOML.

After #49 taught the plugin to recognise flag-valued variables as the
file's geo var, five HIGH findings still fire on basin fx files:
ATTR001 (missing) or ATTR002/ATTR004 (wrong type/constant) for both
_FillValue and missing_value, plus VAR005 (int vs float type check).

Adds _is_flag_variable(ds, var_name) helper and threads a single
is_flag gate through check_Geophysical_Variable:
  - [variable.type] check skipped for flag variables.
  - Attribute rules whose attribute_name is _FillValue or missing_value
    skipped for flag variables.

Existence, dimensions, shape, and all other attribute rules still
apply. Non-flag variables unchanged.

Verified against synthetic reproducer from #59 (5 -> 0 findings for
_FillValue / missing_value / VAR005) and against real AWI-ESM3-4-2-veg-
HR piControl 1851 basin fx file.
@JanStreffing

Copy link
Copy Markdown
Contributor

I found that three fixes have disappeared (but a second pair of eyes confirming this would not hurt)

Confirmed, and thanks for picking this up. I had started a restoring PR from #66 before spotting this one; it turned out to be a strict subset (I had only the two cmip7.py fixes, not 8bd1699), so I closed it in favour of this. My restore of the cmip7.py hunks came out character-for-character identical to yours, which is a decent independent check that both of us reproduced what was originally merged rather than paraphrasing it.

Two things from my end that may be useful for the review.

The regression reached the releases, not just develop. d82d1cf landed 19 Jul and v2.3.3 was tagged two days later, so the release notes credit #49 and #60 by name while the tag contains neither:

ref _is_flag_variable
v2.3.2 absent
v2.3.3 absent (release notes credit it)
v2.3.4 absent
develop / master absent

Verified on real data, not just the synthetic reproducer. A CMIP7 basin fx file from an AWI-ESM3-4-2-veg-HR piControl year (int32, flag_values + flag_meanings, variable_id = "basin"):

develop        geo var = None      1 HIGH  "No geophysical variable detected in the file."
this restore   geo var = 'basin'   0 findings

That is the only HIGH left in our current output, so this PR clears it.

On the extra dec in AVERAGE_CORRECTION_FREQ: that one goes beyond restoration, and it happens to be testable here since we publish decadal means (thkcello, masscello, volcello on tavg-ol-hxy-sea.dec). They are tavg with time: mean, so midpoint is the right resolution for them and I would expect the change to be correct. Happy to run TIME001 across our dec files against this branch and report back if that would help unblock the review.

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.

2 participants