fix: align reconstructed coordinate order with input (+ comparison recipe) - #97
Conversation
The #92 dim-order fix only transposed dims; tsam still returns reconstructed with alphabetically sorted columns, so its coordinate order (e.g. `variable`) differed from `original`'s input order. That left `original.to_dataframe()` and `reconstructed.to_dataframe()` with divergent row orders and made compare()'s concat emit a FutureWarning (join='outer' -> 'exact'). reindex_like(da) after the transpose realigns coordinate order too; the column set is identical, so it only reorders (no NaNs). original, reconstructed, and residuals now line up exactly in values and row order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Shows how to build richer original-vs-reconstructed comparisons directly from the tidy to_dataframe() output (duration curves, Original dotted / Reconstructed solid) — no dedicated plotting helper in the library, keeping it lightweight. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAggregation output now aligns reconstructed coordinates with the input order and labels, suppresses a targeted upstream warning, and adds regression coverage. The getting-started notebook demonstrates dataframe-based original-versus-reconstructed load-duration curves. ChangesAggregation alignment and comparison
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/examples/getting-started.ipynb`:
- Around line 172-188: Update the load-duration curve plotting block after the
`tidy` transformation to use the notebook’s `xarray_plotly` plotting accessor
instead of `plotly.express`, while preserving the existing rank, energy,
variant, variable, line-style, and title configuration.
🪄 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: 50531d44-1c3e-440c-837f-82636e342aea
📒 Files selected for processing (3)
docs/examples/getting-started.ipynbsrc/tsam_xarray/_core.pytest/test_aggregate.py
tsam warns that v3 sorts result columns alphabetically while v4 will follow the input order. tsam_xarray reindexes every output back to the input's coordinate order, so that change is invisible to our users — the warning is noise they can't act on. Filter it narrowly (by message + FutureWarning) around the tsam.aggregate/apply calls. Also drops now-redundant inline comments and consolidates the warnings import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Plot via the notebook's `.plotly.line` accessor instead of plotly.express, matching the rest of the notebook and dropping the extra import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to #93 (now merged): fixes an incomplete part of the dim-order work and adds the comparison-plot recipe we opted for over a first-class plotting method.
Fix — coordinate order (not just dim order)
#93 transposed
reconstructedto the input's dim order, buttsamstill returns it with alphabetically sorted columns, so thevariable(etc.) coordinate order diverged fromoriginal:Consequences:
original.to_dataframe()andreconstructed.to_dataframe()had different row orders, andcompare()'s concat emitted aFutureWarning(join='outer'→'exact') that will become an error in a future xarray.reindex_like(da)after the transpose realigns coordinate order too. The column set is identical, so it only reorders — no NaNs.original,reconstructed, andresidualsnow line up exactly in values and row order. Regression test added (test_non_alphabetical_coord_order_preserved).Docs — comparison recipe (resolves #95 without a plotting method)
Per discussion on #95/#96, we decided not to add a
plot_compare()method (keeps the library lightweight;compare()/to_dataframe()already return plot-ready data). Instead, the getting-started notebook now shows a load-duration-curve comparison built directly from the tidyto_dataframe()output (Original dotted / Reconstructed solid) — the one thing the proposed method offered beyond the existingcompare().plotly.line(...)example.Validation
529 passed / 4 skipped; mypy + ruff clean; notebook executes end-to-end. Closes #95.
🤖 Generated with Claude Code
Summary by CodeRabbit
result.to_dataframe()for comparison workflows.