feat: show what changed inside a NetCDF baseline - #913
Merged
Conversation
A changed `.nc` file previously showed only a size delta and two download links, so a reviewer could not tell a renamed attribute from a numerical regression without opening both blobs by hand. - Adds `netcdf_diff`, which opens both sides with xarray and returns a diff of the ncdump-style header plus one `StatRow` per data variable. - Each row carries min, max, mean and NaN count on both sides, the largest absolute and relative difference, and the number of cells that differ. - NaN counts as equal to NaN, so a masked cell staying masked is not a change. - Rows where something moved are shaded. - Splits the old combined table into a NetCDF section and an "Other files" table. - Declares `xarray`, which the workspace previously only had by accident. - Wraps both tables so a wide one scrolls inside its card, and lets a long file name wrap. Decoding is turned off when opening, so a non-standard calendar or unit cannot fail the report. Stats are whole-array. Per-time or per-level breakdowns are deliberately not here.
- Adds `_variable`, so each side of a data variable is looked up once instead of once for its shape and again for its values. - Extracts `_fetch_pair`, which held the same four lines in `_diff_for` and `_netcdf_for` including the note precedence rule. - Drops the text-flavoured defaults on `_fetch_side`, because `limit` and `oversize` are a pair and a caller that passes one and forgets the other gets a wrong note. - Passes the base side's scale into `_compare` from the reduction `_summarise` already ran, so a large variable is not scanned a third time. - Replaces the three overlapping all-NaN guards in `_compare` with one, and drops the warning filter in `_summarise` that its size check had already made unreachable. - Narrows `_of_kind` to one kind, which is all any call site passes now. - Gives `AnalysedFile.netcdf` the same standing as `text`, with no default. - Adds a `dash` filter so the template no longer decides how an absent shape reads. No behaviour change. The `num` filter is annotated to admit the counts it already formats.
|
Important Approval pendingCodeRabbit has no unresolved comments, but it could not review the latest commit because the review limit was reached. Follow the review guidance in this comment to continue. 📝 WalkthroughWalkthroughThe report now analyses NetCDF files, computes header and variable-level differences, and renders them in separate collapsible sections. Other binary files remain in a separate table. Tests cover analysis, fetching, rendering, and layout behaviour. ChangesNetCDF baseline report
Sequence Diagram(s)sequenceDiagram
participant analyse
participant _fetch_pair
participant netcdf_diff
participant netcdf_block
analyse->>_fetch_pair: fetch NetCDF file sides
_fetch_pair-->>analyse: return fetched paths or note
analyse->>netcdf_diff: calculate header and variable differences
netcdf_diff-->>analyse: return NetcdfDiff
analyse->>netcdf_block: render NetcdfDiff
netcdf_block-->>analyse: render report section
Merge Risk: 🟡 Moderate · up to Some NetCDF changes can be reported inaccurately, while highly compressed files can consume excessive memory. These issues should be fixed before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 56.60% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 53 functions across 4 files. (5 skipped: 5 unsupported.) ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The header was a unified diff, so a reader saw three lines of context around each hunk and nothing else. That names what moved but does not say what the file holds, which is the thing you want when reviewing a baseline you have not seen before. A file whose header did not move showed no header at all. - The header listing now keeps every line and tags the ones that moved, so it doubles as the file's description. The `---`, `+++` and `@@` markers are gone with it. - Adds a Diff and Side by side toggle, so the two headers can be read one next to the other. - `NetcdfDiff` carries `header_old` and `header_new` for that second view, and `header_changed` says whether anything moved. - An unchanged header is marked as such on the summary rather than replaced by a note.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/climate-ref/src/climate_ref/baseline_report/templates/macros.html.j2 (1)
48-48: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for escaped NetCDF header values.
render.pyconfigures Jinja withselect_autoescape(["html", "j2"]), soline.textis escaped in this macro. Add a rendered-header test with a markup payload to protect this setting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 29443150-915a-4267-a2f3-0a63e63b5b37
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
changelog/913.feature.mdpackages/climate-ref/pyproject.tomlpackages/climate-ref/src/climate_ref/baseline_report/analyse.pypackages/climate-ref/src/climate_ref/baseline_report/render.pypackages/climate-ref/src/climate_ref/baseline_report/templates/case.html.j2packages/climate-ref/src/climate_ref/baseline_report/templates/macros.html.j2packages/climate-ref/src/climate_ref/baseline_report/templates/report.csspackages/climate-ref/tests/unit/baseline_report/test_analyse.pypackages/climate-ref/tests/unit/baseline_report/test_render.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
A shaded row said something in the variable changed but not what. Scanning a row of nine old to new pairs to find the one that shifted is work the report can do for the reader. - Adds `Pair`, which holds one statistic on each side and answers whether it moved. - `StatRow` now carries `shape`, `minimum`, `maximum`, `mean` and `nan` as pairs, which drops it from fifteen fields to ten. - The new half of a pair is bold when it differs, and the three diff columns are bold when any cell moved. - A value can be bold while reading the same as its neighbour, because the display rounds to six significant figures and the underlying floats still differ. Also carries an edit to the changelog fragment made outside these changes.
Three findings from the review, all in the statistics. - Keeps the stored dtype when reading a variable. Casting to float first meant an `int64` past 2**53 lost its last digit, so two adjacent values compared equal and a changed cell was reported as unchanged. The mean still accumulates in float64, so a float32 variable reads the same as before. - Reports no maximum difference when a cell moved between NaN and a number. The subtraction gives NaN there, so the finite maximum could read as 0 next to a non-zero cell count. - Bounds what one side may decode to. The fetch cap covers the stored blob, and a compressed file under it can still expand past what the job can hold.
Contributor
Author
|
@coderabbitai review |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A changed
.ncfile in theref test-cases diffreport previously showed only a size delta and two download links, so a reviewer could not tell a renamed attribute from a numerical regression without downloading both blobs and opening them by hand.Builds on #910
Each changed NetCDF file now gets two things: a collapsible diff of its ncdump-style header, and a table with one row per data variable.
netcdf_difftobaseline_report/analyse.py, which opens both sides with xarray and returns the header diff plus oneStatRowper data variable.xarray, which the workspace previously only had through the rootpyproject.tomland transitive dependencies.Decoding is turned off when opening, so a non-standard calendar or unit cannot fail the report. A file over 100 MB is left unopened with a note, and any error opening one becomes a note on that file rather than an exception.
Also fixes a layout bug from #910.
Summary by CodeRabbit
New Features
Improvements