Move PMP to pcmdi_metrics 4.2.1 and enso_metrics 2.0.0 - #901
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (34)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PMP provider now installs a pinned ChangesPMP diagnostics and reference data
Merge Risk: 🟡 Moderate · up to This update installs a PMP fork over the existing conda package to improve diagnostic performance and refreshes diagnostic outputs. Merge readiness depends on confirming that the overlay preserves the intended dependency set; otherwise ENSO processing may fail at runtime. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files. (30 skipped: 30 unsupported.) Full details: Description checkExplanation The description gives a detailed change summary, but it does not follow the repository template. It omits the required “## Description” and “## Checklist” headings and does not confirm the required checklist items. Resolution Add the required “## Description” and “## Checklist” sections. Confirm whether tests were added, whether documentation was added or not applicable, and that the changelog item was added. Also resolve the pcmdi_metrics 4.2.0 versus 4.2.1 version discrepancy between the PR objectives, raw summary, title, and description. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
lewisjared
changed the title
GitHub Actions
Failure
|
This will invalidate the existing pmp runs so we should bump with #898 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/regression/diagnostic.json (1)
85-85: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRe-mint the PMP provenance files with NumPy 2.2.6.
The conda lock file resolves NumPy 2.2.6, but the diagnostic and output provenance files record NumPy 2.0.2. Update all affected annual-cycle provenance fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 1f2b2738-29fc-4662-a18a-c7d5aceeda85
📒 Files selected for processing (59)
changelog/901.fix.mdpackages/climate-ref-pmp/src/climate_ref_pmp/__init__.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/annual_cycle.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/enso.pypackages/climate-ref-pmp/src/climate_ref_pmp/diagnostics/variability_modes.pypackages/climate-ref-pmp/src/climate_ref_pmp/requirements/conda-lock.ymlpackages/climate-ref-pmp/src/climate_ref_pmp/requirements/environment.ymlpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/manifest.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ta/manifest.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ta/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ta/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ts/manifest.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ts/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ts/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip7-ts/manifest.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip7-ts/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/annual-cycle/cmip7-ts/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip6/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/enso_proc/cmip7/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip6/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/enso_tel/cmip7/regression/output.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nam/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nam/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nam/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nam/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nao/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nao/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nao/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-nao/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npgo/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npgo/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npgo/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npgo/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npo/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npo/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npo/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-npo/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pdo/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pdo/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pdo/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pdo/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pna/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pna/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pna/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-pna/cmip7/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-sam/cmip6/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-sam/cmip6/regression/diagnostic.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-sam/cmip7/manifest.jsonpackages/climate-ref-pmp/tests/test-data/extratropical-modes-of-variability-sam/cmip7/regression/diagnostic.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/climate-ref-pmp/src/climate_ref_pmp/init.py
- changelog/901.fix.md
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
lewisjared
changed the title
Second mint failed, holding the conda pin at 3.9.2Mint run 33730826484 bumped the conda environment to
The metric then goes missing from the output bundle, so
Tracked in #906, along with hardening |
…es SVD The fork tracks upstream 4.2.0 and chunks the SVD in the variability modes analysis. This cuts the runtime of the extratropical modes of variability diagnostics by roughly 40x. It goes on with --no-deps over the 3.9.2 conda pin, which cannot move until #906 is resolved.
The chunked SVD in the forked pcmdi_metrics changes the variability modes results, so every PMP baseline has to be re-minted. Bumping all three diagnostics together keeps their versions moving in step.
PMP computes 12 monthly values per statistic in `CalendarMonths`, which the transform step dropped on the way to the scalar bundle. This left `series.json` empty for every annual cycle case. - Extracts one 12 point series per region and statistic, indexed by month number. - Adds a `level` dimension for the 3D pressure-level variables. - Gives the series the same identifying dimensions as the scalars. - Bumps the diagnostic version to 7 and re-mints the four baselines from replay. The native artefacts are unchanged, so the mint reused the stored blobs.
* origin/main: refactor: share one transport between the native and report stores test: cover the report store's error paths fix: keep an unexpected key from tracebacking mid-upload refactor: tidy the report upload after review fix: name the reports bucket ref-baselines-reports fix: report an absent credential instead of a traceback fix: address the review of the report upload feat: publish the baseline diff report and write the PR comment ci: post baseline diff reports from a dedicated workflow fix: measure netcdf differences without losing precision or missingness feat: emphasise the netcdf values that actually moved feat: show the whole netcdf header, with a side by side view chore: rename the changelog fragment to the PR number refactor: tighten the netcdf analysis after the cleanup reviews feat: add netcdf stats to the baseline diff report chore: add a changelog fragment for the mypy 2 bump chore(deps): raise the mypy requirement to match the lockfile chore(deps): bump mistune from 3.3.0 to 3.3.3 chore(deps): update dependency mypy to v2
Regression baseline diff22 test case(s) changed against
|
|
@lee1043 Can you take a look at the diffs in the comment above? There are more differences in the values than I would have expected, but mostly small. The ta annual cycle @ 850 HPa did have some larger changes. https://reports.baselines.climate-ref.org/901/23f40bd0472e/pmp/annual-cycle/cmip6-ta/index.html |
|
@lewisjared thanks for bringing this to my attention and thanks for implementing updated PMP version into REF. It is interesting that the chance is in the annual cycle ta-850 correlation value, while for the other fields correlation remained unchanged. Let me take a look into it and get back to you soon. |
|
@lewisjared my initial guess is it might be related to the change in newer numpy regarding how it handles NaN values -- as only 850 hPa field correlation calculation has been substantially changed. I will continue investigating.. |
|
Hi @lewisjared , We (PMP team) have updated PMP to v4.2.1 that now includes clarification for this #901 (comment), which was followed up in this PCMDI/pcmdi_metrics#1426. In summary, change in the correlation of ta-850 is resulted from "corrected" calculation for correlation when there are NaN value in the field, so I confirm that was expected change. We updated the format to display up to three decimal places, fixing an issue where values like 1.00 or -1.00 were previously rounded to two decimal places. |
* origin/main: Pin the modes of variability CMIP6 test requests to Amon Reword changelog for users Assert the Emon row is excluded from the ts execution Add changelog entry Pin PMP CMIP6 requirements to the Amon table Mint native baselines for the drifted ilamb and pmp cases Fix mint replay tests and clear skip env in fixture Address final review nits Simplify review follow-ups Address review findings Add changelog entry fix: harden test-case fetch and CMIP7 conversion cache Bump version: 0.18.0 → 0.18.1 # Conflicts: # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/manifest.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/regression/diagnostic.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-pr/regression/output.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ts/manifest.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ts/regression/diagnostic.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip6-ts/regression/output.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip7-ts/manifest.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip7-ts/regression/diagnostic.json # packages/climate-ref-pmp/tests/test-data/annual-cycle/cmip7-ts/regression/output.json
The upstream 4.2.1 release contains the chunked variability-modes SVD from the fork. No metric values changed, only the provenance.
lewisjared
changed the title
lewisjared
changed the title
The conda-forge 4.2.1 build needs Python 3.11 or newer, so the env moves to 3.12. enso_metrics 2.0.0 breaks the ENSO feedback metrics on partial calendar years, so the fix from CLIVAR-PRP/ENSO_metrics#98 is pip installed over the top. Provider pip packages now use --force-reinstall, because pip skipped the override when the conda package reported the same version.
The <2.1 pin dates from the CDAT era of enso_metrics and is not needed by the xarray based 2.0.0. Reminted on numpy 2.5.3. ENSO and annual cycle values are identical, the modes shift by less than 1e-13.
|
@lee1043 Can you review again (see the "Regression baseline diff" comment above). I've pulled in enso_metrics 2.0.0 with a small fix as well as latest PMP release. If we can release a v2.0.1, I can include here otherwise we can do a follow up. |
|
Hi @lewisjared, Thanks for the work!
|
|
@lewisjared thanks, just released the new version for |
conda-forge now ships 2.0.1, which carries the partial-calendar-year fix for the ENSO feedback metrics. This drops the pip install of the upstream tag and the pip dependency that existed only to apply it.
The conda spec changed, so the environment hash embedded in the PMP provenance moved. No metric values changed, because 2.0.1 carries the same fix that the forked build applied.
|
Thanks @lee1043 |
2 participants
Moves the PMP conda environment to
pcmdi_metrics4.2.1,enso_metrics2.0.0 and Python 3.12. The 4.2.1 release chunks the SVD in the variability modes analysis, which cuts the runtime of the extratropical modes of variability diagnostics by roughly 40x. Python moves because the conda-forge build of 4.2.1 requires 3.11 or newer.enso_metrics2.0.0 breaks the ENSO feedback metrics on any time window that does not cover whole calendar years (#906). The fix is in CLIVAR-PRP/ENSO_metrics#98 and is pip installed over the conda package with--no-depsuntil it is released. This follows the pattern the ESMValTool provider already uses.Two things had to change for that override to work.
pip, because the 4.2.1 solve no longer pulls it in andmicromamba run pipotherwise picks up a pip outside the environment.--force-reinstall, because pip silently skipped the fork when the conda package reported the same version.Diagnostic.versionis bumped on all three PMP diagnostics, and all 22 baselines are minted on linux-64 with the final environment. Closes #906.Things worth a close look.
cmip6-tais the interesting one.cor_xywas between 0.86 and 0.97 depending on season and is now roughly 0.996 in every season, which reads as a fixed misalignment rather than drift.tais the only 3D case here.enso_metrics2.0.0 is a full migration off CDAT onto xarray. Most shifts are a few percent. The largest is anenso_telHadISST statistic going from 32.7 to 36.6. The output bundle also lists the metrics that were not computed for lack of input variables as empty entries.Summary by CodeRabbit
New Features
Bug Fixes
Tests