feat: first-class original-vs-reconstructed comparison on AggregationResult - #93
Conversation
Resolves the two papercuts in #92 for eyeballing aggregation quality. - Dim order: tsam emits `reconstructed` as `(time, *cluster_dims)`, which disagreed with `original` whenever the input placed `time` last. Transpose `reconstructed` to the input's dim order so `original`, `reconstructed`, and `residuals` always align for `.to_dataframe()`/plotting. - `AggregationResult.compare(**sel)`: stacks `original` and `reconstructed` along a new `variant` dim (`["original", "reconstructed"]`) on the original time axis, plottable directly via `color="variant"` with no melt step. - `AggregationResult.to_dataframe(**sel)`: tidy long-form DataFrame for direct handoff to a plotting library. - Docs: getting-started "Reconstructed vs original" now uses the compare() one-liner in place of the manual concat/assign_coords boilerplate. Closes #92 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesAggregation comparison utilities
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AggregationResult
participant xarrayDataArray
participant pandasDataFrame
AggregationResult->>xarrayDataArray: compare with optional selections
xarrayDataArray-->>AggregationResult: variant-stacked comparison
AggregationResult->>pandasDataFrame: to_dataframe conversion
pandasDataFrame-->>AggregationResult: reset-index long-form table
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tsam_xarray/_result.py (1)
148-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Examples:instead ofExample:for Google-style docstrings.As per coding guidelines, the project uses Google-style docstrings. The Google Python Style Guide specifies that the section header should be plural (
Examples:), even if there is only a single example provided.💡 Proposed tweak
- Example: + Examples: >>> agg.compare(variable="solar").plotly.line(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tsam_xarray/_result.py` around lines 148 - 151, Change the docstring section heading in the affected result aggregation documentation from “Example:” to “Examples:” while leaving the existing example content unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tsam_xarray/_result.py`:
- Around line 180-181: Update the DataArray-to-dataframe conversion around
combined.name so the selected data column name falls back to "value" when it
equals the added "variant" dimension coordinate name. Pass this collision-safe
name to combined.to_dataframe(), while preserving existing names for all other
inputs.
---
Nitpick comments:
In `@src/tsam_xarray/_result.py`:
- Around line 148-151: Change the docstring section heading in the affected
result aggregation documentation from “Example:” to “Examples:” while leaving
the existing example content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54056ffc-734b-4e7c-a626-188c2c09899a
📒 Files selected for processing (4)
docs/examples/getting-started.ipynbsrc/tsam_xarray/_core.pysrc/tsam_xarray/_result.pytest/test_aggregate.py
When the input DataArray is named "variant", to_dataframe() reused that as the value-column name and clashed with the added "variant" dim on insert (ValueError: cannot insert variant, already exists). Fall back to "value" in that case; all other input names are preserved. Also fix the docstring section heading Example -> Examples (Google style). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Resolves both papercuts in #92.
Papercut 1 — consistent dim order
tsamemitsreconstructedas(time, *cluster_dims), which disagreed withoriginalwhenever the input placedtimelast (the case in the issue).reconstructedis now transposed to the input's dim order, sooriginal,reconstructed, andresidualsalways share dim order and line up directly in.to_dataframe()/plotting. No-op fortime-first inputs.Papercut 2 — comparison helpers
AggregationResult.compare(**sel)→ a singleDataArraystacked along a newvariantdim (["original", "reconstructed"]) on the original time axis. Plots directly viacolor="variant"— noconcat/melt. Optional coord selection, e.g.compare(variable="solar").AggregationResult.to_dataframe(**sel)→ tidy long-formDataFrame(variant,time, cluster/slice dims, value column) for direct handoff to a plotting lib.Naming
Dim is
variant, not the issue's suggestedrepresentation— the latter is already tsam'sSegmentConfig(representation=...)and is serialized into the clustering JSON, so reusing it would overload the term.serieswas rejected for colliding with "time series". Values stayoriginal/reconstructedto match the existing.original/.reconstructedattributes.Docs
getting-started.ipynb"Reconstructed vs original" now uses thecompare()one-liner in place of the manualconcat/assign_coordsboilerplate the issue quotes.Tests
Added
TestDimOrderConsistency(3) andTestCompare(4). Full suite green (mypy + ruff clean, notebook executes end-to-end).Closes #92
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation