Can we make 1e-6 a parameter "abs_tolerance" of the method with a default value? Or it could be a private attribute of the class. Either way, I don't like seeing them scattered across various if-statements.

Can we make 1e-6 a parameter "abs_tolerance" of the method with a default value? Or it could be a private attribute of the class. Either way, I don't like seeing them scattered across various if-statements.

Done - that was in the code before refactor.

I've typically seen "homogeneous" rather than "homogenous", e.g. https://en.wikipedia.org/wiki/Homogeneous_coordinates

Yup - fixed.

I would add a note here that any object with an __array__ method will work, as long as the array shape matches.

Is there a policy for how to say that in a docstring? I am used to array-like for that - as in anything that can become an array with np.array(param) - but perhaps that is not your tradition.

Hmm, good point! 😅 I am used to thinking as array-likes being:

• numpy arrays
• lists and tuples (including nested lists)
• other arrays, such as dask arrays

I guess what I was trying to say is that I wouldn't think of skimage.transform.AffineTransform as an "array-like" without further prodding. So I think "array-like" in the type section is fine, but I would add a sentence to the description like:

Note that any object with an `__array__` method [1]_ that returns a matrix
with the correct dimensions can be used as input here. This includes all
subclasses of `skimage.transform.ProjectiveTransform`, for example.

[...]

References
----------
.. [1]: https://numpy.org/doc/stable/user/basics.interoperability.html#using-arbitrary-objects-in-numpy

Ah yes - I see what you mean - I've added your change. Do you think this should go everywhere we accept an array-like for a transformation matrix? Such as the class constructors?

Personally, I prefer classes to have all their expected attributes even if they can be null, rather than not have the attribute. ie, I would make sure that self.params is at least set to None. (I'd consider using dataclasses in this refactor.)

Since params is a public attribute, I'd consider this a breaking change...

The only purpose of this line is to avoid confusing warnings when object construction fails - and the only situation in which the object would lack self.params is if object construction fails. So I don't think this code could break anything out there; it would only improve the repr displayed when the object generates an error during construction.

avoid confusing warnings when object construction fails

under what circumstances can construction fail but still return an instance?

under what circumstances can construction fail but still return an instance?

This was happening in the tests, when the constructor failed, and the error message displayed a repr of the half-initialized object. In that case you'd get two errors, one displaying the repr - not relevant to the problem - and the other for the actual problem in initialization. Because I found that distracting, I made this change to prevent the first error. But I wouldn't die on a hill to keep it - it shouldn't get triggered in ordinary practice.

@matthew-brett what do you think about setting params=None first thing in the constructor so we don't have to hasattr here?

I would rather avoid putting params=None at the top of all the __init__s, because people are more likely to read that code, and be confused by it - when it is only solving a problem they would not normally see. So my preference order would be:

1. The current fix in the __repr__ / __nice__ function.
2. No fix at all, just let the double error happen in the rare case where initialisation fails and we generate a __repr__ of the result.
3. params=None in all __init__s.

Has there been much discussion about this rule? I'm not sure how I feel about it, and could be convinced that it's good, but AffineTransform(scale=2, dimensionality=3) seems like an ok thing to me a priori?

I personally slightly prefer a more explicit API such as

AffineTransform(scale=(2, 2, 2))

that can do away with an additional parameter dimensionality=. That simplifies input validation and documentation.

I think I added it when making transforms nD-compatible because it was possible before to do AffineTransform().estimate(...), so we want to still be able to do that for higher dimensions, as in AffineTransform(dimensionality=3).estimate(...). If we get rid of dimensionality, this would become (presumably) AffineTransform.identity(ndim=3).estimate(...), which is clunky.

We could instead make estimate a staticmethod, since it doesn't use starting parameters anyway. But that's a breaking change. Perhaps something to make an issue for skimage2?

I presume that AffineTransform().estimate(...) was always a 2D identity transform? If so, there is no change if we remove dimensionality from the constructor.

One could argue about which of AffineTransform(dimensionality=3).estimate(...) or AffineTransform.identity(3).estimate(...) is more clunky - I personally find the second easier to read, because I have to translate the first into "Make an identity transform of dimension 3" in my head. But the problem with the dimensionality= constructor call is that, in order to have this convenience in using the constructor for the identity, we have a constructor where one argument is nearly always redundant or incorrect, if any other parameters are specified.

We could instead make estimate a staticmethod, since it doesn't use starting parameters anyway. But that's a breaking change. Perhaps something to make an issue for skimage2?

Are you suggesting that we have the constructor with no input arguments generate a self.params=None not-initialized object, and then, if estimate finds such an object, it works out what identity transform to initialize with?

We could make a constructor such that it would be possible to tform = AffineTransform.from_estimate(src, dst).

Of course that would just boil down to a slight polishing of:

@classmethod
def from_estimate(cls, src, dst):
    obj = cls.identity(src.shape[1])
    if not obj.estimate(src, dst):
        raise SomeError('Estimation failed')
    return obj

That doesn't seem unreasonable. And it would make it less common to need to construct the identity transform explicitly.

Well, for 2D Transform().estimate is already working and the way we teach it, so why not just make that work with other dimensions as well?

It's a bit unreasonable to expect a transform to function before either initializing it with parameters, or estimating those parameters.

I think getting rid of dimensions is fine, as is introducing identity.

But, ok, I'll let @jni take another look. I'm pretty sleep deprived atm.

Chiming in late to the conversation, but I'd also prefer making use of classmethods. Both in a vacuum and with the current API Transform.from_estimate seems like a good idea for already stated reasons. Transform.estimate() feels unnecessarily confusing, and I'd be fine with deprecating Transform().estimate(). It's always been slightly bugging me that the API allows for an "empty" transform and a readonly instance (think frozen dataclass) seems like a very good fit to me.

I am still puzzled by the staticmethod idea. Given that the callable produces a class, and needs to know its own class, surely it should be classmethod.

Sorry, you are right. Technically it needs to know its own class but, looking at the code, literally every class has its own estimate method, so it could be hardcoded, which is where my brain was going. But I think that is not as nice as using cls(params), so indeed classmethod is the way to go.

It's too fancy and magical (see advice above). It will make even experienced developers do a double take, and try and work out what is going on.

This would only be a temporary state, because instance.estimate() would be deprecated. So I don't think it's a big deal.

I don't think estimate is the right name for a classmethod (see below) that returns a new transform instance. I think it needs to be from_something. I personally find from_correspondence ugly, and still prefer from_estimation - it doesn't worry me that you pass points rather than an estimation, that will be obvious from inspecting the signature, and not surprising.

I acknowledge that from_something is the most common pattern for classmethod naming, but it's certainly not a hard and fast rule, and for me tf = AffineTransform.estimate(src, dst) is the most natural incantation by a long shot. I think @stefanv also prefers it but I'll let him speak for himself when he's awake. 😂 And again, I am in no way advocating to keep both the instance and the class methods around, I'm thinking about what to do in the future. Having the instance method turns out to make no sense so imho keeping it is out of the question.

I find AffineTransform.from_estimation awkward, and would potentially even prefer registration.from_points(src, dst, tf_class=AffineTransform).

Incidentally I don't see why identity(n) gets to avoid the from prefix. 😉

I'm on board with identity and I'm on board with dropping dimensionality.

@jni - can you live with estimated as the class constructor name? If so, I think we're very close ...

I can live with from_estimate as the class constructor name.

And this feels super weird... imho either we allow scalars, or we don't, I don't see why the dimensionality needs to be "implied"?

I concur, either we deprecate implicit broadcasting of scalars to the appropriate shape everywhere (my preference), or we keep dimensionality= around.

See below.

My reasoning for this change was that I found it hard to grok the meaning of e.g dimensionality=3 in a constructor call like SimilarityTransform(np.eye(4), dimensionality=3), or SimilarityTransform(rotation=(0.1, 0.2), dimensionality=3). That is - it seemed to be, nearly always, either redundant, or to require an error on construction, when the dimensionality does not match the inputs.

On further reflection, it became clear that the dimensionality=3 parameter was almost invariably used to construct an identity transform - as in SimilarityTransform(dimensionality=3). So this leads us to the odd situation where, for a huge majority of calls (actually, I think all but one in the test suite) dimensionality, when present, is redundant, a source of error, or intended to construct an identity transform.

Put another way, in the vast majority of cases, when passing any non-default argument to the constructor, dimensionality is implied by the arguments, and dimensionality is redundant, or should cause an error because it does not match.

I thought this was confusing and unsatisfying as an API, so I planned to, in due course, remove the dimensionality parameter entirely from the constructor API, and ask users to write SimilarityTransform.identity(3) instead, for the vast majority of cases where dimensionality should be specified.

This left only one valid use-case for non-identity transforms, non-default dimensionality, and that was the scalar scale case you've referred to. As I say - I think that was used only once in the whole test suite. Of course, it will only be useful specifically in the 3 or greater dimension case - that is - the only place where dimensionality makes sense, for not-default input arguments is specifically e.g. SimilarityTransform(scale=4, dimensionality=3).

So it seemed better to me, from the point of view of the API, to plan, in due course, to remove dimensionality from the constructor, because it is nearly always redundant, and for the only case where it was not redundant, ask users to specify the dimensionality by SimilarityTransform(scale=(4, 4, 4)) instead. This, along with the .identity constructor, allows us to remove a huge number of redundant uses of dimensionality, with very small loss of convenience, for this scalar scale case.

Or to put it yet another way - I think the current class constructor API is a fusion of two different constructors, which the current API fuses into one constructor:

# Transform from parameters.
SimilarityTransform(rotations=(0.1, 0.2, 0.3))
# Identity transform.
SimilarityTransform(dimensionality=3)

leading to confusing calls like:

# In user code, but also occurs in the scikit-image codebase.
SimilarityTransform(rotations=(0.1, 0.2, 0.3), dimensionality=3)

I think this should be, in due course:

# Transform from parameters.
SimilarityTransform(rotations=(0.1, 0.2, 0.3))
# This will raise an error, because `dimensionality` will not be an argument to constructor.
SimilarityTransform(rotations=(0.1, 0.2, 0.3), dimensionality=3)
# Identity transform.
SimilarityTransform.identity(3)

Therefore, it separates the two APIs.

Thanks for the rationale @matthew-brett. See this comment re APIs — I think AffineTransform(dimensionality=3).estimate(src, dst) reads better than AffineTransform.identity(3).estimate(src, dst). However, even better is AffineTransform.estimate(src, dst), with the dimensionality implicit in the shapes of src and dst. Therefore, I think forge ahead with your proposal, but with an eye towards making estimation a static method. Thoughts?

typo + singular (thinking, there's not even a single other parameter)

I really like this idea! ❤️