MAINT: make ellipse fitting forward compatible by ev-br · Pull Request #8054 · scikit-image/scikit-image

ev-br · 2026-02-14T12:34:03Z

Description

NumPy plans to change it's linalg.eig routine to always return complex results, instead of the current complex or real if eigenvalues happen to be on the real axis, numpy/numpy#30411

The only effect on scikit-image AFAICS, is in the ellipse estimation routines, which require that eigenvalues are real. In fact, the story seems to be somewhat convoluted:

some datasets produce non-zero imaginary parts, which break EllipseModel.fit, Estimating Ellipse Sometimes fails due to np.linalg.eig Casting to complex #7013
the current failure mode is a TypeError from an in-place modulo operation, phi %= np.pi, where phi is constructed from eigenvalues/eigenvectors
Estimating Ellipse Sometimes fails due to np.linalg.eig Casting to complex #7013 (comment) asks for a reproducer, and Make eig/eigvals always return complex eigenvalues numpy/numpy#29000 (comment) has some discussion of potential fixes/enhancements

This PR makes a minimal fix to make EllipseModel.fit both backwards and forwards compatible: take the real part of eigenvalues/eigenvectors explicitly, and raise a TypeError if either of them has non-zero imaginary parts.

This is backwards compatible because

if all eigenvalues are on the real line, we explicitly take the real part---even if numpy does it for us, .real.real is a no-op
if there are non-zero imaginary parts, it raises the same TypeError, with a different error message: instead of implementation details, the new message asks to report the reproducer, per Estimating Ellipse Sometimes fails due to np.linalg.eig Casting to complex #7013 (comment)

It is forwards compatible for the numpy change: the output of eig will always be complex with zero imaginary parts, and the fix here takes the real part.

Checklist

A descriptive but concise pull request title
Docstrings for all functions
Unit tests
A gallery example in ./doc/examples for new features
Contribution guide is followed

Release note

This is not user-visible.

coderabbitai · 2026-02-14T12:36:47Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 96af9446-bcc8-43eb-a0d1-2688d736c955

📥 Commits

Reviewing files that changed from the base of the PR and between 18b217b and 0ef31f0.

📒 Files selected for processing (1)

src/skimage/measure/fit.py

📝 Walkthrough

Walkthrough

Adds a validation in EllipseModel._estimate to detect non-real eigenvalues/eigenvectors from the generalized eigen-decomposition; if any imaginary parts are present it raises a ValueError, otherwise it proceeds using the real parts to compute ellipse parameters.

Changes

Cohort / File(s)	Summary
Eigenvalue validation `src/skimage/measure/fit.py`	Added a check in `EllipseModel._estimate` to detect complex eigenvalues/eigenvectors. If imaginary components exist, the method raises `ValueError` with a user-facing message; otherwise eigenvalues/eigenvectors are converted to their real parts and computation continues.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'MAINT: make ellipse fitting forward compatible' is concise, descriptive, clearly summarizes the main change (making ellipse fitting forward compatible with NumPy's planned changes), and is well under 80 characters.
Description check	✅ Passed	The description is directly related to the changeset, providing detailed context about NumPy's planned change, the current issue with EllipseModel.fit, and explaining the fix's backwards and forwards compatibility.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

coderabbitai · 2026-02-14T12:36:50Z

src/skimage/measure/fit.py

+        # https://github.com/scikit-image/scikit-image/issues/7013
+        if np.any(eig_vals.imag != 0) or np.any(eig_vecs.imag != 0):
+            raise TypeError(
+                "Non-real eigvalues/eigvec are not currently supported. Please "
+                "report the reproducer to "
+                "https://github.com/scikit-image/scikit-image/issues/7013"
+            )
+        eig_vals = eig_vals.real
+        eig_vecs = eig_vecs.real


⚠️ Potential issue | 🟠 Major

raise TypeError breaks the established error-handling contract of _estimate.

Every other failure path in this method (lines 859, 874, 901, 924) returns via _warn_or_msg(…, warn_only), which lets from_estimate wrap it in a FailedEstimation. Raising TypeError here will instead propagate as an unhandled exception, crashing RANSAC loops and any other caller that expects failed estimations to be signaled gracefully.

Additionally, the exact != 0 check on imaginary parts is fragile. Floating-point arithmetic on M can introduce tiny imaginary components (e.g., O(1e-16)) that are numerical noise, not a genuine problem. This would crash the caller on data that could otherwise be fitted successfully—which contradicts the forward-compatibility goal.

Consider using a tolerance-based check and returning through _warn_or_msg instead:

Proposed fix

# https://github.com/scikit-image/scikit-image/issues/7013 - if np.any(eig_vals.imag != 0) or np.any(eig_vecs.imag != 0): - raise TypeError( - "Non-real eigvalues/eigvec are not currently supported. Please " - "report the reproducer to " - "https://github.com/scikit-image/scikit-image/issues/7013" + if not np.allclose(eig_vals.imag, 0) or not np.allclose(eig_vecs.imag, 0): + return _warn_or_msg( + "Non-real eigenvalues/eigenvectors are not currently supported. " + "Please report the reproducer to " + "https://github.com/scikit-image/scikit-image/issues/7013", + warn_only=warn_only, ) eig_vals = eig_vals.real eig_vecs = eig_vecs.real

🧰 Tools

🪛 Ruff (0.15.0)

[warning] 909-913: Avoid specifying long messages outside the exception class

(TRY003)

This bot comment is incorrect. Emitting TypeError is strictly backwards compatible because of gh-7013 and

In [1]: import numpy as np In [2]: np.complex128(3) % np.pi --------------------------------------------------------------------------- TypeError Traceback (most recent call last) Cell In[2], line 1 ----> 1 np.complex128(3) % np.pi TypeError: unsupported operand type(s) for %: 'numpy.complex128' and 'float'

EDIT: As much as I normally am against equality comparison in floating point, and how much I agree that in general it's vastly superior to use a tolerance etc etc, w.imag == 0 is what numpy did: https://github.com/numpy/numpy/blob/main/numpy/linalg/_linalg.py#L1503

I think it's fine to check for 0 here. I think it's right that we don't expect to see that error type (never used to before the new NumPy nightly), but once you run into complex numbers here you're in trouble and we should abort anyway.

I modified the code a bit to use np.isreal and np.all.

stefanv · 2026-03-06T05:10:10Z

src/skimage/measure/fit.py

+        # https://github.com/scikit-image/scikit-image/issues/7013
+        if np.any(eig_vals.imag != 0) or np.any(eig_vecs.imag != 0):
+            raise TypeError(
+                "Non-real eigvalues/eigvec are not currently supported. Please "
+                "report the reproducer to "
+                "https://github.com/scikit-image/scikit-image/issues/7013"
+            )
+        eig_vals = eig_vals.real
+        eig_vecs = eig_vecs.real


I think it's fine to check for 0 here. I think it's right that we don't expect to see that error type (never used to before the new NumPy nightly), but once you run into complex numbers here you're in trouble and we should abort anyway.

stefanv · 2026-03-06T05:10:50Z

src/skimage/measure/fit.py

+        # https://github.com/scikit-image/scikit-image/issues/7013
+        if np.any(eig_vals.imag != 0) or np.any(eig_vecs.imag != 0):
+            raise TypeError(
+                "Non-real eigvalues/eigvec are not currently supported. Please "
+                "report the reproducer to "
+                "https://github.com/scikit-image/scikit-image/issues/7013"
+            )
+        eig_vals = eig_vals.real
+        eig_vecs = eig_vecs.real


I modified the code a bit to use np.isreal and np.all.

coderabbitai

🧹 Nitpick comments (1)

src/skimage/measure/fit.py (1)
907-911: The forward-compatibility fix looks correct.

The approach using np.isreal() and extracting .real parts handles both the current NumPy behavior (sometimes real, sometimes complex) and the planned future behavior (always complex with zero imaginary parts for real results). Per the maintainer's clarification, raising an exception here is backwards compatible since complex values would have caused a TypeError at phi %= np.pi anyway.

One minor observation: the PR objectives mention raising an error that asks users to report reproducers to issue #7013, but the current error message is quite terse. Consider adding the issue link for better user guidance:
         if not (np.all(np.isreal(eig_vals)) and np.all(np.isreal(eig_vecs))):
-            raise ValueError("Expected real eigenvalues and -vectors")
+            raise ValueError(
+                "Expected real eigenvalues and -vectors. Please report a "
+                "reproducer to https://github.com/scikit-image/scikit-image/issues/7013"
+            )
This would help users encountering edge-case data that produces non-real eigenvalues to contribute reproducers for further investigation.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 13c65e6f-d466-4249-a0fb-e659d072bd7d

📥 Commits

Reviewing files that changed from the base of the PR and between 77f5fb8 and 18b217b.

📒 Files selected for processing (1)

src/skimage/measure/fit.py

* origin/main: (27 commits) Move `_shared` to `_skimage2` (#8102) Port `pad_footprint` & `mirror_footprint` to `skimage2` (#8094) Add rescaling API to skimage2 (#8075) Move `skimage2` implementation into `_skimage2` (#8093) Undo (double) mirroring in `ski2.morphology.dilation` (#8060) Grammar (#8091) Move `ensure_spacing` into `skimage2` (#8067) Ensure that `skimage2` does not eagerly import `skimage` (#8087) Use lazy-loader 0.5 (#8080) MAINT: make ellipse fitting forward compatible (#8054) Avoid deprecated assign to ndarray.shape (#8020) Avoid circular import in `feature/corner.py` (#8077) Turn off coderabbit auto-labeling & label checks (#8070) Fix GIL being re-enabled by C++ extensions (#8059) Fix conventions following docstub and misc. (#8055) Port grayscale morphology operators to skimage2 (#8046) Add revised `peak_local_max` to `skimage2` (#8039) Allow read-only arrays as input to remap (#7535) Add `prescale` parameter to "blob functions" (#7858) Try to make coderabbit respect exisiting type labels (#8042) ...

MAINT: make ellipse fitting forward compatible

77f5fb8

coderabbitai bot added the 🔧 type: Maintenance Refactoring and maintenance of internals label Feb 14, 2026

coderabbitai bot reviewed Feb 14, 2026

View reviewed changes

ev-br mentioned this pull request Feb 14, 2026

ENH: linalg: return complex arrays from eigvals/eigvecs numpy/numpy#30411

Merged

Reformulate slightly

18b217b

stefanv approved these changes Mar 6, 2026

View reviewed changes

coderabbitai bot reviewed Mar 6, 2026

View reviewed changes

Add back message to report data

0ef31f0

stefanv merged commit b9b377c into scikit-image:main Mar 6, 2026
25 checks passed

stefanv added this to the 0.27 milestone Mar 6, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MAINT: make ellipse fitting forward compatible #8054

MAINT: make ellipse fitting forward compatible #8054
stefanv merged 3 commits intoscikit-image:mainfrom
ev-br:eig_cmplx

ev-br commented Feb 14, 2026

Uh oh!

coderabbitai bot commented Feb 14, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Feb 14, 2026

Uh oh!

ev-br Feb 14, 2026 •

edited

Loading

Uh oh!

stefanv Mar 6, 2026

Uh oh!

stefanv Mar 6, 2026

Uh oh!

stefanv Mar 6, 2026

Uh oh!

stefanv Mar 6, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ev-br commented Feb 14, 2026

Description

Checklist

Release note

Uh oh!

coderabbitai bot commented Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

ev-br Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanv Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

stefanv Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

stefanv Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

stefanv Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Feb 14, 2026 •

edited

Loading

ev-br Feb 14, 2026 •

edited

Loading