Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support array-likes consistently in geometric transforms #6270

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Feb 28, 2022

Description

closes #4383

As-is some transform types support array-like inputs (e.g. the translation argument to AffineTransform), but most did not. This PR attempts to make all __call__, inverse, __init__ and estimate calls support array-like inputs. A number of test cases were parametrized to test with list-of-list inputs for matrix, params and src/dst coordinates.

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • There is a bot to help automate backporting a PR to an older branch. For
    example, to backport to v0.19.x after merging, add the following in a PR
    comment: @meeseeksdev backport to v0.19.x
  • To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
    can be removed and then added again. The benchmark output can be checked in
    the "Actions" tab.

…rray-like inputs

As-is some transform types support array-like inputs, but most did not
resolves scikit-imagegh-4383
@grlee77 grlee77 added ⏩ type: Enhancement Improve existing features 🩹 type: Bug fix Fixes unexpected or incorrect behavior labels Feb 28, 2022
@grlee77 grlee77 added this to the 1.0 milestone Feb 28, 2022
Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @grlee77 😉

Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting only a small change in one of the tests!

@@ -245,11 +246,21 @@ def test_fundamental_matrix_residuals():
assert_almost_equal(tform.residuals(src, dst)**2, [0, 0.5, 2])


def test_fundamental_matrix_forward():
@pytest.mark.parametrize('array_like_input', [False, True])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit confusing for me at first read, because an array is a fortiori array-like... 🙃
But I admit that not_array_but_array_like_input would be too long!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkcor I'd be in the not_array_but_array_like_input team! 😂 It wouldn't bite us in the back in the future.
What do you think, @grlee77?

skimage/transform/tests/test_geometric.py Outdated Show resolved Hide resolved
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@pep8speaks
Copy link

pep8speaks commented Apr 1, 2022

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-14 14:48:46 UTC

@alexdesiqueira alexdesiqueira merged commit a3daff5 into scikit-image:main Jun 14, 2022
@alexdesiqueira
Copy link
Member

Thank you @grlee77!

@lagru lagru modified the milestones: 0.21, 0.20 Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior ⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when using the inverse of a Transform with params given as np.matrix
6 participants