Conversation
|
@Muriel23 and @paolomassa reviews would be great |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
==========================================
+ Coverage 92.43% 92.86% +0.42%
==========================================
Files 14 16 +2
Lines 1600 1724 +124
==========================================
+ Hits 1479 1601 +122
- Misses 121 123 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new spectral component imaging capability to xrayvision, enabling decomposition of multi-energy visibility measurements into per-component visibilities (per Stiefel et al. 2025), with accompanying tests and documentation so users can apply existing imaging algorithms to each recovered component.
Changes:
- Introduces
xrayvision.spectral.vis_spectral_componentsimplementing the spectral decomposition via weighted linear least squares. - Adds pytest coverage for core correctness and input validation.
- Publishes user-facing docs + a new gallery example demonstrating two-component separation, and documents the change in the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
xrayvision/spectral.py |
New implementation of spectral component visibility decomposition. |
xrayvision/tests/test_spectral.py |
New tests validating decomposition, normalization handling, and error cases. |
examples/component_imaging.py |
New end-to-end gallery example on synthetic data showing separated imaging. |
docs/reference/spectral.rst |
New API reference page for the spectral submodule. |
docs/reference/index.rst |
Adds spectral to the reference toctree. |
changelog/103.feature.rst |
Changelog entry announcing the new function. |
.codespellrc |
Updates codespell ignore words list to accommodate new identifiers/terms. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if any( | ||
| v.u.shape != ref.u.shape or not apu.quantity.allclose(v.u, ref.u) or not apu.quantity.allclose(v.v, ref.v) | ||
| for v in vis[1:] | ||
| ): |
There was a problem hiding this comment.
🟡 Changes recommended
Input validation, phase-center consistency, and normalized output units need correction before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (6) — in code that hasn't changed since the last review.
xrayvision/spectral.py:88
- Non-finite fractions pass this validation, and NumPy can propagate them through
solvewithout raisingLinAlgError, returning component visibilities full of NaNs. Reject non-finite fraction values before constructing the normal equations.
xrayvision/spectral.py:105 - The decomposition combines complex visibilities as though they use one spatial reference, but only checks the sampled coordinates. Inputs with different phase centers therefore produce phase-shifted, incorrect components that are labeled with
ref.phase_center; require matching phase centers too.
xrayvision/spectral.py:118 - A zero, negative, NaN, or infinite total flux cannot normalize a visibility. These values currently create infinities/NaNs or trigger a misleading uncertainty error; validate that normalization is finite and positive before dividing.
This issue also appears on line 120 of the same file.
xrayvision/spectral.py:152
- When
normalizationis a total flux invis_unit, the solved values representV/Fand are dimensionless, as documented above. Reattachingvis_unitmislabels both the component values and uncertainties, causing downstream images to report physical flux units for normalized morphology; return dimensionless quantities in the normalized branch and update the normalization test accordingly.
xrayvision/tests/test_spectral.py:86 - This assertion locks in a flux unit for
V/F, although dividing the visibility by a same-unit total flux produces a dimensionless relative visibility. Once the implementation preserves the correct dimensional result, assert againstdimensionless_unscaledhere.
examples/component_imaging.py:14 - The two independent clauses run together here. End the citation sentence and introduce the STIX documentation separately.
xrayvision/spectral.py:121
- This positivity test does not reject NaN or infinite uncertainties. Either value makes the weights or propagated variance non-finite, allowing invalid component visibilities to be returned; require finite positive uncertainties.
if np.any(sigma_array <= 0):
raise ValueError("amplitude_uncertainty must be positive for all visibilities.")
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
| "`fractions` is not full rank, e.g. one energy bin's fractional contributions are a " | ||
| "linear combination of the others, or there are fewer independent energy bins than " | ||
| "spectral components." |
Implements the method described in Stiefel et al. 2025 (A&A, 704, A316, doi:10.1051/0004-6361/202557373)