218 image analysis and image plotting helper refactor by dmehring · Pull Request #219 · casangi/astroviper

dmehring · 2026-04-02T15:52:29Z

This PR builds on PR 217. PR 217 should be merged before this one is.

… assumptions

…new docstring on that and dask config

Normalise DataArrays with named x/y dims to (x, y) axis order before resolving coordinates and labels, so callers using the common astronomy (y, x) storage layout get correct plot axes and labels. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

WCSAxes derives correct RA/Dec labels from the WCS object itself. Only call set_axislabel() in the WCS branch when the caller explicitly passed a string coordinate selector as a custom label. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Non-monotonic world-coordinate values in coord are intentionally allowed; only the synthetic pixel-index axis must be strictly increasing. Update the Returns and Notes sections to reflect the actual preconditions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 11 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T18:52:07Z

src/astroviper/distributed/model/component_models.py

 import dask.array as da

+from ...utils._gaussian_math import (
+    FWHM2SIG as _FWHM2SIG,


_FWHM2SIG is imported from astroviper.utils._gaussian_math but never used in this module. Please drop the unused import (or use it) to avoid lint failures / dead imports.

Suggested change

FWHM2SIG as _FWHM2SIG,

Copilot · 2026-04-02T18:52:08Z

src/astroviper/utils/plotting.py

+        def _coord_from_spec(spec, dim_name: str, axis_size: int) -> np.ndarray:
+            if spec is None:
+                if dim_name in data.coords:
+                    coord_values = np.asarray(data.coords[dim_name].values, dtype=float)
+                else:
+                    coord_values = np.arange(axis_size, dtype=float)
+            elif isinstance(spec, str):
+                if spec not in data.coords:
+                    raise ValueError(f"Coordinate {spec!r} not found in DataArray.")
+                coord_values = np.asarray(data.coords[spec].values, dtype=float)
+            else:
+                coord_values = np.asarray(spec, dtype=float)


_resolve_plot_coords() coerces DataArray coordinate values to dtype=float even when show_world_axes=False (pixel/imshow mode), which can raise for legitimate non-numeric coords (e.g., datetime64 or string labels) even though those coords are not needed for pixel plotting. Consider only resolving/casting x/y coordinate vectors when show_world_axes=True and wcs is None, or avoid forcing dtype=float unless the coordinates are actually used for pcolormesh/world-axis rendering.

tests/benchmark/distributed/image_analysis/benchmark_multi_gaussian2d_fit.py

Copilot · 2026-04-02T18:52:08Z

tests/benchmark/distributed/image_analysis/benchmark_multi_gaussian2d_fit.py

+def make_scene_via_component_models(
+    nx: int,
+    ny: int,
+    components: list[dict],
+    *,
+    offset: float = 0.1,
+    noise_std: float = 0.02,
+    seed: int | None = None,
+    coords: bool = True,
+    angle: str = "math",  # "math" | "pa" | "auto" — same semantics as astroviper's model
+    x_world: tuple[float, float] = (0.0, 1.0),
+    y_world: tuple[float, float] = (0.0, 1.0),
+) -> xr.DataArray:


make_scene_via_component_models() annotates components as list[dict], but the implementation treats it as either a list-of-dicts (single-channel) or a list-of-lists-of-dicts (multi-channel) via isinstance(components[0], dict). Tightening the type annotation (or simplifying the logic to match the annotation) would make the intended input contract clearer and avoid confusion for benchmark users.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add need_coords flag to _resolve_plot_coords and set it only for the pcolormesh (show_world_axes=True, no WCS) branch where coordinate vectors are actually consumed. Other rendering modes no longer attempt a float cast on DataArray coordinates, so datetime64 or string labels no longer raise needlessly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ssian2d_fit.py Only set envs if run directly Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ssian2d_fit.py remove dead code Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

https://github.com/casangi/astroviper into 218-image-analysis-and-image-plotting-helper-refactor

…dels The function accepts both list[dict] (single-channel) and list[list[dict]] (multi-channel); update the annotation and docstring to reflect both forms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 11 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T19:38:13Z

src/astroviper/utils/plotting.py

+    values = np.asarray(data)
+    if values.ndim != 2:
+        raise ValueError(
+            "data must be a 2D array-like object with shape (nx, ny) for plotting."
+        )
+
+    if not need_coords:
+        return (
+            values,
+            np.arange(values.shape[0], dtype=float),
+            np.arange(values.shape[1], dtype=float),
+        )


_resolve_plot_coords skips validation of x_coords/y_coords when need_coords=False, which means passing a string selector with a NumPy input (or in the WCS branch) is silently ignored instead of raising as documented. Consider validating x_coords/y_coords types up front (or at least rejecting str selectors when data is not an xarray.DataArray) so mistakes don’t slip through depending on show_world_axes/wcs.

tests/unit/distributed/image_analysis/test_multi_gaussian2d_fit.py

tests/benchmark/distributed/image_analysis/benchmark_multi_gaussian2d_fit.py

String selectors were silently ignored when need_coords=False (imshow and WCS branches). Now validate them upfront in both the DataArray and plain-array paths so bad inputs always raise regardless of rendering mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ssian2d_fit.py remove dead code Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

https://github.com/casangi/astroviper into 218-image-analysis-and-image-plotting-helper-refactor

Copilot

Pull request overview

Copilot reviewed 7 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/astroviper/utils/plotting.py

Copilot · 2026-04-02T19:59:39Z

src/astroviper/utils/plotting.py

+        # Validate string selectors upfront so bad inputs always raise,
+        # regardless of whether coordinate vectors will be consumed.
+        for spec, label in ((x_coords, "x"), (y_coords, "y")):
+            if isinstance(spec, str) and spec not in data.coords:
+                raise ValueError(f"Coordinate {spec!r} not found in DataArray.")
+
+        if not need_coords:
+            # Coordinate vectors will not be used by the caller; skip the
+            # DataArray coord lookup and float cast to avoid raising on
+            # non-numeric coordinates (e.g. datetime64, string labels).


In the wcs plotting branch, need_coords is false and x_coords/y_coords are used only as optional axis-label overrides, but this code still requires the strings to exist in data.coords and raises otherwise. That makes it impossible to use x_coords='ra'/y_coords='dec' as pure labels unless the DataArray happens to expose coords with those exact names. Consider skipping this validation when need_coords=False (or splitting the API into separate x_label/y_label parameters) so WCS label overrides don’t depend on DataArray coordinate presence.

Suggested change

# Validate string selectors upfront so bad inputs always raise,

# regardless of whether coordinate vectors will be consumed.

for spec, label in ((x_coords, "x"), (y_coords, "y")):

if isinstance(spec, str) and spec not in data.coords:

raise ValueError(f"Coordinate {spec!r} not found in DataArray.")

if not need_coords:

# Coordinate vectors will not be used by the caller; skip the

# DataArray coord lookup and float cast to avoid raising on

# non-numeric coordinates (e.g. datetime64, string labels).

if not need_coords:

# Coordinate vectors will not be used by the caller; skip the

# DataArray coord lookup and float cast to avoid raising on

# non-numeric coordinates (e.g. datetime64, string labels) and

# to allow string x_coords / y_coords to be used as label-only

# overrides by callers such as the WCS plotting path.

Copilot · 2026-04-02T19:59:39Z

src/astroviper/utils/plotting.py

+        if not need_coords:
+            # Coordinate vectors will not be used by the caller; skip the
+            # DataArray coord lookup and float cast to avoid raising on
+            # non-numeric coordinates (e.g. datetime64, string labels).
+            return (
+                np.asarray(data.values),
+                np.arange(nx, dtype=float),
+                np.arange(ny, dtype=float),
+            )


np.asarray(data.values) forces immediate materialization of the full DataArray (and will compute Dask-backed arrays) even in modes where callers might reasonably expect plotting to accept already-realized NumPy inputs only. Since other parts of this PR explicitly avoid touching .values to preserve laziness, it would be more consistent to use np.asarray(data.data) plus an explicit np.asarray(... ) conversion at the plotting call sites (where materialization is truly required), or document clearly that generate_plot() always realizes the full array for plotting.

…=False In the WCS and imshow branches, x_coords/y_coords strings are label overrides and do not require a matching DataArray coordinate. Move the existence check inside the need_coords=True path where coords are actually looked up. The pcolormesh path (world axes, no WCS) still raises on missing coord names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…boundary _resolve_plot_coords now returns data.data (raw underlying array) instead of calling np.asarray(data.values), keeping lazy Dask arrays un-computed until generate_plot explicitly materializes with np.asarray() at the single rendering boundary. Document the behaviour in both docstrings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 11 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T20:19:55Z

src/astroviper/utils/plotting.py

+def _resolve_plot_coords(
+    data,
+    x_coords: Optional[Union[str, np.ndarray]] = None,
+    y_coords: Optional[Union[str, np.ndarray]] = None,
+    *,
+    need_coords: bool = True,
+) -> tuple[np.ndarray, np.ndarray, np.ndarray]:
+    """


_resolve_plot_coords is annotated to return tuple[np.ndarray, np.ndarray, np.ndarray], but in the xarray.DataArray path it returns data.data, which can be a Dask array (or other array-like) rather than a NumPy ndarray. This makes the type hint inaccurate and can trip static type checking; consider widening the first element’s type (e.g., np.ndarray | dask.array.Array or a more general ArrayLike) to match the docstring’s “NumPy or Dask” behavior.

The first element may be a NumPy ndarray or a Dask array depending on the DataArray's backing storage; Any accurately reflects this while the docstring explains the contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tting-helper-refactor 218 image analysis and image plotting helper refactor

dmehring added 30 commits March 19, 2026 12:05

updates to gaussian fitting and supporting modules

ecf2679

updated docs

9227f80

large refactor for maintainability

c558640

Tighten multi-Gaussian bounds semantics

8244ca2

Validate ordered principal-axis bounds

20fad7e

add optional title

82089d6

mostly doc updates and more forgiving param handling

9955694

warning for malchunked dask arrays, lots of comments about axis order…

9331db3

… assumptions

allow caller to specify axes convention for unlabled arrays, lots of …

e1843e5

…new docstring on that and dask config

various updates to fitter, plotting

d4aea79

trivial change to rerun CI

6f53b6b

note explicitly pixel coord conventoin

795655b

code cleanup

6b41a95

black

a2e0681

mitigate xy, yx amibiguity

98cb3a8

add docstrings to tests

28491cf

update stale docs

35f6330

bug fix interpolating decreasing axis

b542de7

doc clean up

02f82b8

test clean up

eb0f832

black

5d380fd

flat array FWHM convert to sigma correctly

485c39c

Fix multi-Gaussian plotting and flat-init edge cases

f913810

Clarify component model raw-axis docs

a796979

black

f098141

Fix point-source Dask chunk alignment

a0e6807

better initial guesses for quicker convergence

4b2fc82

Fix multi-Gaussian call provenance metadata

d750701

Gate multi-Gaussian world outputs on coords

506d172

Fix interpolation helper monotonicity check

6eb25e6

dmehring and others added 3 commits April 2, 2026 14:20

dmehring requested a review from Copilot April 2, 2026 18:44

Copilot started reviewing on behalf of dmehring April 2, 2026 18:45 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

dmehring and others added 6 commits April 2, 2026 15:18

Drop unused _FWHM2SIG import from component_models

d62d0f2

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Update tests/benchmark/distributed/image_analysis/benchmark_multi_gau…

38013ac

…ssian2d_fit.py Only set envs if run directly Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tests/benchmark/distributed/image_analysis/benchmark_multi_gau…

581d99a

…ssian2d_fit.py remove dead code Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch '218-image-analysis-and-image-plotting-helper-refactor' of

c8f8522

https://github.com/casangi/astroviper into 218-image-analysis-and-image-plotting-helper-refactor

dmehring requested a review from Copilot April 2, 2026 19:28

Copilot started reviewing on behalf of dmehring April 2, 2026 19:31 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

dmehring and others added 4 commits April 2, 2026 15:45

Update tests/benchmark/distributed/image_analysis/benchmark_multi_gau…

ab655ee

…ssian2d_fit.py remove dead code Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update tests/benchmark/distributed/image_analysis/benchmark_multi_gau…

e3f2375

…ssian2d_fit.py remove dead code Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Merge branch '218-image-analysis-and-image-plotting-helper-refactor' of

0ff4dfd

https://github.com/casangi/astroviper into 218-image-analysis-and-image-plotting-helper-refactor

dmehring requested a review from Copilot April 2, 2026 19:51

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Copilot started reviewing on behalf of dmehring April 2, 2026 20:01 View session

dmehring and others added 2 commits April 2, 2026 16:04

dmehring requested a review from Copilot April 2, 2026 20:09

Copilot started reviewing on behalf of dmehring April 2, 2026 20:15 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Jan-Willem merged commit c5b0948 into main Apr 2, 2026
3 of 19 checks passed

e-koch pushed a commit that referenced this pull request Apr 6, 2026

Merge pull request #219 from casangi/218-image-analysis-and-image-plo…

9053cb6

…tting-helper-refactor 218 image analysis and image plotting helper refactor

Conversation

dmehring commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmehring commented Apr 2, 2026 •

edited

Loading