Add model init params, deprecate `params` arguments by matthew-brett · Pull Request #7789 · scikit-image/scikit-image

matthew-brett · 2025-05-05T17:53:20Z

As discussed over at #7771 (comment) - the PR:

Deprecates no-argument constructor calls to the model classes such as LineModelND;
Deprecate .params attributes of models. Instead set model-specific attributes: origin, direction for LineModelND; center, radius for CircleModel, center, ax_lens, theta for EllipseModel.
Deprecates use of model-initiating extra parameters (params) in predict* calls.

There are a couple of changes for consistency and clarity in error type from params=None passed to the predict etc methods - see release note below.

For discussion

All three models have a params attribute, with differing interpretation of the elements:

LineModelND : sequence of two length N 1D arrays, elements origin and direction.
CircleModel : length 3 vector with elements center_x, center_y, radius.
EllipseModel: length 5 vector with elements center_x, center_y, first_ax_len, second_ax_len, theta (angle of first axis).

I wonder whether it would be better to record these input parameters directly on the instance, rather than munging them into params as in:

LineModelND : attributes origin and direction.
CircleModel : attributes center (length 2 array), radius.
EllipseModel: attributes center (length 2 array), ax_lens (length 2 array), theta (scalar).

I say this because at the moment (in this PR) we ask for input parameters according to the attributes above, but do not record them as attributes in the same way, but rather in the concatenated params form, which might be confusing.

Release note

For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.

In `skimage.measure`, deprecate use of model constructor calls without arguments
leaving an uninitialized instance (for example `CircleModel()`). This applies to
`CircleModel`, `EllipseModel`, and `LineModelND`. Instead prefer input arguments
to define instances (for example `CircleModel(center, radius)`). This follows on
from prior deprecation of the `estimate` method, which had implied the need for
the no-argument constructor, of form `cm = CircleMoldel(); cm.estimate(data)`.
{label="API"}

In `skimage.measure`, deprecate `.params` attributes of the models `CircleModel`,
`EllipseModel`, and `LineModelND`.  Instead set model-specific attributes: 
`origin, direction` for `LineModelND`; `center, radius` for `CircleModel`,
`center, ax_lens, theta` for `EllipseModel`.
{label="API"}

In `skimage.measure`, deprecate use of `params` arguments to `predict*` calls of 
model objects. This applies to `CircleModel`, `EllipseModel`, and `LineModelND`.
We now ask instead that the user provide initialization equivalent to the
`params` content in the class construction. For example, prefer 
`cm = CircleModel((2, 3), 4); x = cm.predict_x(t)` to
`cm = CircleMoldel(); x = cm.predict_x(t, params=(2, 3, 4))`).
{label="API"}

Deprecate `skimage.measure.fit.BaseModel`; after we expire the other `*Model*`
deprecations, there is no work for an ancestor class to do.
{label="API"}

Raise a `ValueError` instead of a `TypeError` in `CircleModel`, `EllipseModel`, and `LineModelND` in `skimage.measure`. This applies when failing  to pass a value for
`params` (or passing `params=None`) to `predict` methods of an uninitialized
transform.
{label="Enhancement"}

jni · 2025-05-09T04:21:32Z

I wonder whether it would be better to record these input parameters directly on the instance, rather than munging them into params as in:

I like your proposals @matthew-brett.

stefanv · 2025-05-19T17:38:32Z

Discussed on call with @lagru and @matthew-brett: will make .params read-only (error on write), and deprecate it in favor of direct access of each parameter on class (circle.center etc.).

In general, ask for input parameters to models such as Circle, etc. Deprecate use of no params to these models. This is sensible because we use `from_estimate` for the most sensible no-params case.

Deprecate use of `.params`. Use `center`, `radius` etc for shape model classes.

matthew-brett · 2025-07-10T17:30:22Z

Ok - I deprecated .params on the shape models, and added the explicit attributes such as center, direction etc. The ransac routine does not require a .params attribute for models to be valid, so no changes needed in ransac.

This one is now ready for review.

Naming seemed clearer to me.

* origin/main: Update import convention in certain gallery examples (scikit-image#7764) Refactor fundamental matrix scaling (scikit-image#7767)

matthew-brett · 2025-07-11T13:22:10Z

I also added notes on edits to expire the deprecations.

lagru · 2025-07-11T14:22:49Z

skimage/measure/fit.py

+    @deprecate_func(
+        deprecated_version=_PARAMS_DEP_START,
+        removed_version=_PARAMS_DEP_STOP,
+        hint='`params` attribute deprecated; use object attributes directly',


I don't think the hint adds anything that the default message is not already including?

Suggested change

hint='`params` attribute deprecated; use object attributes directly',

Except, the suggestion is to use e.g. center, rather than params. Can you think of a better way of phrasing this?

I'm guessing that's because this decorator is supposed to work for all models? If we added independent deprecation machinery for each of the models, it would be a lot easier to use a specific hint here, saying "use origin/center/radius".

Could also remove some complexity. Because each implementation only has to work for one model instead of all?

I've pasted and edited the params property into all the model classes. Yes, maybe that is easier to follow.

lagru · 2025-07-11T14:25:34Z

skimage/measure/fit.py

        return tf if msg is None else FailedEstimation(f'{cls.__name__}: {msg}')


+class _ParamsBaseModel(BaseModel):


Is there reason for not adding the methods of this class directly to the BaseModel? As far as I can see, BaseModel isn't used anywhere besides as a subclass for this one.

I noticed it popped up a couple of times in grep of the code-base, and it is in the public API - so I was insecure about messing with it. Should I investigate further?

I checked usage with my local IDE and it seemed like merging the two classes should have no negative implications for unrelated usages. But I or IDE could be wrong.

I guess it doesn't matter that BaseModel gets a few more methods - I've fused _ParamsBaseModel into BaseModel.

lagru · 2025-07-11T14:29:10Z

skimage/measure/fit.py

+        # from the ``_estimate`` methods, to the respective ``from_estimate``
+        # class methods.
+        with catch_warnings(action='ignore'):
+            tf = cls()


In retrospect this might have been easier if we used the classmethod from_estimate to implement estimate instead of the other way around were we have to awkwardly call create an instance to call a method in a classmethod. 😅

But not worth changing now I think.

skimage/measure/fit.py

lagru · 2025-07-11T14:50:54Z

skimage/measure/fit.py

+# * Delete `_params2init_values` methods.
+# * Delete `params` property of LineModelND.
+# * Rename `_arg_init` methods to `__init__`.
+# * Clean `params` parsing from `predict_*` methods.


After starting to review this – even with this comment – completing the deprecation seems like a lot. I suspect, anybody trying to complete this deprecation 1-2 years from now will have a hard time completing this cleanly.
And since we are potentially planning to update the coordinate conventions too, there is potentially be another complicating factor that will come on top of this.

I guess what I want to say is, that my "spider sense" is tingling a lot when looking at all the back and forth between base and subclasses and decorators.

Do you think we could simplify this? I'd be totally on board with a lot more duplication if that can help with that endeavor. E.g. a straightforward __init__ for each of the models, that does what's required instead of _deprecate_no_args.

In my mind, an ideal deprecation mostly implements the new API as straightforward as possible. An then there is an added layer on top – ideally a decorator – that handles the backward compatibility. To complete it, then mostly only deleting the decorator is required.

The current implementation seems like something in between, mixing the old and the new implementation of the API as well as the deprecation machinery.

That said, you are more familiar here, so if you say this level of complexity is generally necessary, than I'll go with it. 😅

I hesitate to say this is the best way to do it - but I did think quite hard about it, as well as expiring the deprecation, as you see. I don't myself think it would be all that hard - most of it is deleting stuff.

The problem here is that I want the init signature to show only the new signature, and not *args, **kwargs, as it must if we don't do this kind of dance. This is to prepare the user best for the new API. And the rest of the complexity it having to deal simultaneously with the old params args, and the deprecation of the no-argument instantiation calls.

@lagru

Fuse BaseModel and _ParamsBaseModel. From @lagru review.

* origin/main: Setup stub creation with docstub in CI (scikit-image#7802)

stefanv

Approved, pending an update to TODO.txt.

Thanks @matthew-brett!

skimage/measure/tests/test_fit.py

@stefanv

From review by @stefanv. Co-authored-by: Stefan van der Walt <[email protected]>

Add TODO on deprecation procedure.

TODO.txt

Co-authored-by: Stefan van der Walt <[email protected]>

matthew-brett · 2025-07-16T09:06:00Z

pre-commit.ci autofix

lagru · 2025-07-16T09:30:42Z

I'll have a look at the docstub failure.

Just use `~.DEPRECATED` for the deprecated parameters. This has the added benefit of type checkers flagging the usuage of the parameter.

Must have accidentally skipped that somehow during the most recent merge.

skimage/measure/fit.py

@lagru

From clarification by @lagru

skimage/measure/fit.py

lagru · 2025-07-16T13:06:08Z

skimage/measure/fit.py

 class BaseModel:
-    def __init__(self):
-        self.params = None
+    def __init_subclass__(self):


Off-topic: I guess this works to discourage subclassing BaseModel.

We could also flat out refactor to _fit.py and deprecate any access to skimage.measure.fit. But I'm thinking that's something we could do a bit later during the transition to skimage2. Maybe with a some meta-programming shenanigans that warns for all access to modules that are not exposed in our HTML documentation (synonymous with the tree defined by __all__).

lagru · 2025-07-16T13:12:15Z

skimage/measure/fit.py

+@_deprecate_no_args
+class LineModelND(_BaseModel):


I'm wondering how the interaction between these three parts affects users that subclass LineModelND. E.g. will they get a useful warning and do we need to worry about this?

Some local testing implies that this seems to work, e.g. when called with super().estimate(...). So probably fine. 🤞

skimage/measure/fit.py

matthew-brett · 2025-07-16T13:26:14Z

I'm planning to merge this soon - @lagru - anything you see as a blocker as of now?

lagru · 2025-07-16T13:32:33Z

@matthew-brett, still reviewing this. I'll merge once ready and if I don't find anything critical. Hope that's okay.

matthew-brett · 2025-07-16T13:34:04Z

Let me know when you're happy, I'll write a suitable merge message.

and fix typo: "by passing not input arguments to constructor" -> "by passing no input arguments to constructor"

lagru

Thanks @matthew-brett. Feel welcome to merge.

lagru · 2025-07-16T13:56:29Z

In addition to the merge summary we should also update the release notes in the PR description. I'd like to split them into distinct items like in this example here https://github.com/scientific-python/changelist?tab=readme-ov-file#writing-pull-request-summaries. I can do that after the merge, if you haven't done so already then.

matthew-brett · 2025-07-16T14:29:25Z

Thanks @lagru - merged - please do update the release notes - sorry I did not do that.

lagru · 2025-11-29T10:58:57Z

I tried to make the release notes a bit more end user friendly in the context of our release notes. Will be updated in #7967.

matthew-brett marked this pull request as draft May 5, 2025 17:53

matthew-brett added the 📜 type: API Involves API change(s) label May 5, 2025

matthew-brett force-pushed the model-init-params branch from 1050ab9 to cc1768e Compare May 8, 2025 22:24

Require init params for models, with deprecation

91e99e8

In general, ask for input parameters to models such as Circle, etc. Deprecate use of no params to these models. This is sensible because we use `from_estimate` for the most sensible no-params case.

matthew-brett force-pushed the model-init-params branch from cc1768e to 91e99e8 Compare July 9, 2025 14:55

Use explicit shape model parameters

6d6e792

Deprecate use of `.params`. Use `center`, `radius` etc for shape model classes.

matthew-brett marked this pull request as ready for review July 10, 2025 17:29

matthew-brett added 5 commits July 11, 2025 10:09

Rename init_params to init_values

e5c0086

Naming seemed clearer to me.

Add guide for expiring deprecation.

2d2119d

Docstring for params property.

815a4e2

LineModelND params was a tuple, restore.

e482b6c

Test LineModelND params.

8c009e0

matthew-brett force-pushed the model-init-params branch from ed906ad to 8c009e0 Compare July 11, 2025 09:45

matthew-brett added 2 commits July 11, 2025 14:19

Merge remote-tracking branch 'origin/main' into model-init-params

60f9bd2

* origin/main: Update import convention in certain gallery examples (scikit-image#7764) Refactor fundamental matrix scaling (scikit-image#7767)

Apply Ruff formatting.

a4ca3fa

lagru reviewed Jul 11, 2025

View reviewed changes

skimage/measure/fit.py Outdated Show resolved Hide resolved

lagru reviewed Jul 11, 2025

View reviewed changes

matthew-brett and others added 4 commits July 12, 2025 00:20

Separate params property for each model class

c7212b7

Fuse BaseModel and _ParamsBaseModel. From @lagru review.

Renamed _chk_ to _check_ in method name.

6964b30

Merge remote-tracking branch 'origin/main' into model-init-params

52eb9ac

* origin/main: Setup stub creation with docstub in CI (scikit-image#7802)

Merge branch 'main' into model-init-params

e409d00

stefanv approved these changes Jul 16, 2025

View reviewed changes

stefanv reviewed Jul 16, 2025

View reviewed changes

skimage/measure/tests/test_fit.py Outdated Show resolved Hide resolved

matthew-brett and others added 2 commits July 16, 2025 08:27

Update skimage/measure/tests/test_fit.py

1c9d5ad

From review by @stefanv. Co-authored-by: Stefan van der Walt <[email protected]>

Deprecate BaseModel

bc591d7

Add TODO on deprecation procedure.

matthew-brett force-pushed the model-init-params branch from 2cb15bf to bc591d7 Compare July 16, 2025 08:55

stefanv reviewed Jul 16, 2025

View reviewed changes

TODO.txt Outdated Show resolved Hide resolved

Update TODO.txt

27a620b

Co-authored-by: Stefan van der Walt <[email protected]>

'[pre-commit.ci 🤖] Apply code format tools to PR'

ee90efd

lagru added 4 commits July 16, 2025 12:09

Address docstub errors in measure/fit.py

29c25da

Just use `~.DEPRECATED` for the deprecated parameters. This has the added benefit of type checkers flagging the usuage of the parameter.

Merge branch 'main' into ghpr/7789_model-init-params

232d00d

Re-add shell: bash to conform with main branch

487d3d6

Must have accidentally skipped that somehow during the most recent merge.

Rename to helper funcs to _assert_ellipse_*

84b8f50

lagru reviewed Jul 16, 2025

View reviewed changes

skimage/measure/fit.py Outdated Show resolved Hide resolved

Rename ax_lens to axis_lengths

127d0d1

From clarification by @lagru

lagru reviewed Jul 16, 2025

View reviewed changes

skimage/measure/fit.py Show resolved Hide resolved

lagru reviewed Jul 16, 2025

View reviewed changes

skimage/measure/fit.py Show resolved Hide resolved

Avoid duplication of "Version 2.2" section

80b76b0

and fix typo: "by passing not input arguments to constructor" -> "by passing no input arguments to constructor"

lagru approved these changes Jul 16, 2025

View reviewed changes

matthew-brett merged commit 83a65ac into scikit-image:main Jul 16, 2025
25 checks passed

stefanv added this to the 0.26 milestone Jul 16, 2025

ericpre mentioned this pull request Nov 1, 2025

Failure with scikit-image 0.26.0rc0.dev0+git20251025 pyxem/pyxem#1174

Closed

		return tf if msg is None else FailedEstimation(f'{cls.__name__}: {msg}')


		class _ParamsBaseModel(BaseModel):

Uh oh!

Conversation

matthew-brett commented May 5, 2025 • edited by lagru Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For discussion

Release note

Uh oh!

jni commented May 9, 2025

Uh oh!

stefanv commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthew-brett commented Jul 10, 2025

Uh oh!

matthew-brett commented Jul 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lagru Jul 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matthew-brett commented Jul 16, 2025

Uh oh!

lagru commented Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

lagru Jul 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthew-brett commented Jul 16, 2025

Uh oh!

lagru commented Jul 16, 2025

Uh oh!

matthew-brett commented Jul 16, 2025

Uh oh!

lagru left a comment

Choose a reason for hiding this comment

Uh oh!

lagru commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matthew-brett commented Jul 16, 2025

Uh oh!

lagru commented Nov 29, 2025

Uh oh!

Reviewers

Assignees

matthew-brett commented May 5, 2025 •

edited by lagru

Loading

stefanv commented May 19, 2025 •

edited

Loading

lagru Jul 11, 2025 •

edited

Loading

lagru Jul 16, 2025 •

edited

Loading

lagru commented Jul 16, 2025 •

edited

Loading