-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add example to FundamentalMatrixTransform class #6863
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a small suggestion but looks good otherwise. If you want you could also add some brief descriptions to separate the logical parts of the example like in bc6baeb.
Co-authored-by: Lars Grüter <[email protected]>
@lagru I have incorporated your suggestions. |
>>> import skimage as ski | ||
>>> tform_matrix = ski.transform.FundamentalMatrixTransform() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> import skimage as ski | |
>>> tform_matrix = ski.transform.FundamentalMatrixTransform() | |
>>> tform_matrix = FundamentalMatrixTransform() |
(iirc this explicit import is unnecessary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's not necessary for the tests to pass I think complete imports should be part of each doctests. That makes them easier to copy and also more explicit in a few ways. Also I recall that in some edge cases doctest gets confused if functions or classes share a name of a submodule. Using ski.module.some_object
avoids this.
It's also more consistent since we have to include import skimage as ski
in doctests that uses API from other modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Marianne Corvellec <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving since this already is a great improvement. Though personally, I'd prefer for #6863 (comment) / 85d7910 to be reverted.
@ana42742, I am curious. Do you remember where you got the numbers from? |
I'm also in favour of reverting #6863 (comment) / 85d7910. As per #7020 (comment), I'll wait until Dec 14. If we don't hear from @ana42742 until then, I'll make the change and approve the PR. |
This reverts commit 85d7910.
Description
Addresses a task in #6808.
Resolves a task inI have added an example to the FundamentalMatrixTransform Class in skimage.transform module.
This comprehensive example demonstrates its three methods,
estimate
,residuals
andinverse
as well.Checklist
./doc/examples
(new features only)./benchmarks
, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.example, to backport to v0.19.x after merging, add the following in a PR
comment:
@meeseeksdev backport to v0.19.x
run-benchmark
label. To rerun, the labelcan be removed and then added again. The benchmark output can be checked in
the "Actions" tab.