8000 Fix `axline` for slopes <= 1E-8. Closes #28386 by kyracho · Pull Request #28754 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Fix axline for slopes <= 1E-8. Closes #28386 #28754

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
Closed
wants to merge 10 commits into from
Closed

Fix axline for slopes <= 1E-8. Closes #28386 #28754

wants to merge 10 commits into from

Conversation

kyracho
Copy link
Contributor
@kyracho kyracho commented Aug 23, 2024

PR summary

This pull request addresses #28386

Changes:

  1. Manually set the tolerance for np.isclose as suggested by Andresporcruz in the dormant pull request Fix axline for slopes < 1E-8 #28431.

  2. Write a test in test_lines.py to ensure axline doesn't plot horizontal lines for slopes <= 1E-8.

A demonstration that shows the issue has been fixed:

import matplotlib
import matplotlib.pyplot as plt 
matplotlib.use('TkAgg')
plt.axline(xy1=(0, 0), slope=1E-8)
plt.ylim(-1E-8,1E-8)
plt.xlim(-1,1)
plt.show()
test

PR checklist

github-actions[bot]
github-actions bot reviewed Aug 23, 2024
View reviewed changes
Copy link
@github-actions github-actions bot 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 for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@kyracho kyracho changed the title Fix axline for slopes <= 1E-8. Addresses issue #28386 Fix axline for slopes <= 1E-8. Closes #28386 Aug 23, 2024
@oscargus
Copy link
Member
oscargus commented Aug 23, 2024 •
edited
Loading

Thank you for this! However, I do not think this is the right fix. Now, there will be issues with slope = 1e-9?

I think a better fix is to detect the case here:

matplotlib/lib/matplotlib/lines.py

Lines 1516 to 1519 in 3eda5a3

else:
# one point and a slope were given
x1, y1 = points_transform.transform(self._xy1)
slope = self._slope

so set a variable manual_slope to False before

matplotlib/lib/matplotlib/lines.py

Line 1502 in 3eda5a3

if self._xy2 is not None:
and then set it to True in the section above.

Use this to detect if a manual slope is provided by rewriting if np.isclose(slope, 0): to if not manual_slope and np.isclose(slope, 0) or slope == 0: as I guess the point is to avoid changing a manually set slope to 0 rather than have a strict limit at 1e-8.

(I am sure that one can get that approach to behave weird as well, but assuming that the use case is to have a small slope with a suitable small range, I think it is a more robust approach.)

@oscargus oscargus mentioned this pull request Aug 23, 2024
Closed
@kyracho kyracho closed this Aug 27, 2024
@kyracho
Copy link
Contributor Author
kyracho commented Aug 27, 2024

I closed the PR because I mistakenly initiated it from the main branch of my forked repository. I'll be creating a new PR from a separate branch after revisiting the development workflow guide. Thanks for being patient with me and my first PR.

@kyracho kyracho mentioned this pull request Aug 28, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kyracho @oscargus
0