10000 Use matplotlib's infinite axline to demonstrate hough transform by stefanv · Pull Request #4877 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Use matplotlib's infinite axline to demonstrate hough transform #4877

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 12, 2021
Merged

Use matplotlib's infinite axline to demonstrate hough transform #4877

merged 2 commits into from
Feb 12, 2021

Conversation

stefanv
Copy link
Member
@stefanv stefanv commented Jul 31, 2020 •
edited
Loading

This requires matplotlib 3.3.

It does prevent problems when np.sin(angle) is zero.

@pep8speaks
Copy link
pep8speaks commented Jul 31, 2020 •
edited
Loading

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

Line 113:54: E226 missing whitespace around arithmetic operator

Comment last updated at 2021-02-12 22:13:43 UTC

@grlee77
Copy link
Contributor
grlee77 commented Jul 31, 2020

I think this actually needs Matplotib >= 3.3!. When I tried with 3.1.1 I got AttributeError: 'AxesSubplot' object has no attribute 'axline'

There are axhline and axvline in 3.0, but not axline.

@grlee77
Copy link
Contributor
grlee77 commented Jul 31, 2020

Matplotlib 3.0 is nearly two years old, so that seems reasonable to require. Matplotlib 3.3, though, was just released 15 days ago! Do you think it is worth having two code paths in the example until we make 3.3 a requirement or should we hold off on this change for now?

@tacaswell
Copy link
Contributor

Yeah, this is a 3.3 feature.

@stefanv stefanv force-pushed the hough-tf-mpl-axline branch from 1fc7a75 to 447ddc4 Compare July 31, 2020 21:58
@stefanv
Copy link
Member Author
stefanv commented Jul 31, 2020

Woops, meant to write 3.3 ;)

@stefanv
Copy link
Member Author
stefanv commented Jul 31, 2020

I think using Arch linux has messed with me.

What?! Not running latest? Get at it!

@hmaarrfk
Copy link
Member
hmaarrfk commented Aug 1, 2020

Can we change the dependency for the docs instead?
https://github.com/scikit-image/scikit-image/blob/master/requirements/docs.txt

We already require a newer version of matplotlib for docs than for the build.

@emmanuelle
Copy link
Member

I agree with Mark here: if we bump a dependency for a version which is only 2 weeks old, it should be the docs dependencies, not the package ones.

@stefanv
Copy link
Member Author
stefanv commented Aug 2, 2020

Sure, that's fine. But I may hold off until 3.3 is widely available to users, otherwise this won't run for most readers.

@sciunto sciunto added 🔍 Monitor Depends on an external condition(s); monitor it 🤔 Planning 📄 type: Documentation Updates, fixes and additions to documentation ⏩ type: Enhancement Improve existing features labels Aug 6, 2020
sciunto
sciunto reviewed Aug 6, 2020
View reviewed changes
@sciunto sciunto mentioned this pull request Aug 10, 2020
Closed
@stefanv stefanv mentioned this pull request Jan 15, 2021
Merged
@madphysicist
Copy link
Contributor

FWIW, I'm all for this change.

@sciunto sciunto added action: mrg+1 and removed 🔍 Monitor Depends on an external condition(s); monitor it 🤔 Planning labels Jan 15, 2021
@jni
Copy link
Member
jni commented Jan 15, 2021

I'm happy to merge ASAP if we change this to happen with the docs requirement rather than the default ones.

@jni jni mentioned this pull request Jan 18, 2021
Closed
@stefanv stefanv force-pushed the hough-tf-mpl-axline branch from 4c9b02f to a997e18 Compare February 12, 2021 01:17
@stefanv
Copy link
Member Author
stefanv commented Feb 12, 2021

OK, should be good to go.

@rfezzani rfezzani self-assigned this Feb 12, 2021
rfezzani
rfezzani approved these changes Feb 12, 2021
View reviewed changes
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.

I just wonder if you meant matplotlib version decrease, otherwise I am 👍 for the introduced changes. Thank you @stefanv

requirements/default.txt Outdated
@@ -1,6 +1,6 @@
numpy>=1.16.5
scipy>=1.2.3
matplotlib>=3.0.3
matplotlib>=3.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops, no, thanks for catching that; fixed.

@stefanv stefanv force-pushed the hough-tf-mpl-axline branch from 3e40419 to ea2d558 Compare February 12, 2021 22:13
@rfezzani rfezzani merged commit 9346b03 into scikit-image:master Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sciunto sciunto sciunto approved these changes

@rfezzani rfezzani rfezzani approved these changes

Assignees

@rfezzani rfezzani

Labels
⏩ type: Enhancement Improve existing features 📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@stefanv @pep8speaks @grlee77 @tacaswell @hmaarrfk @emmanuelle @madphysicist @jni @sciunto @rfezzani
0