Skip to content

Fix: [Bug]: Setting norm by string doesn't work for hexbin #28105 #28106

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
Apr 29, 2024
Merged

Fix: [Bug]: Setting norm by string doesn't work for hexbin #28105 #28106

merged 2 commits into from
Apr 29, 2024

Conversation

clementgilli
Copy link

@clementgilli clementgilli commented Apr 19, 2024 •
edited
Loading

PR summary

The problem was that the methods on the norm were called directly on a string instead of the object itself.
So I put this block after `collection.set_norm(norm) in order to call the norm's getter.

# autoscale the norm with current accum values if it hasn't been set
        if norm is not None:
            if collection.norm.vmin is None and collection.norm.vmax is None:
                collection.norm.autoscale()

Seems to solve the issue.

PR checklist

github-actions[bot]
github-actions bot reviewed Apr 19, 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.

@tacaswell
Copy link
Member

Thank you for working on this! I think you need to do atleast two more things:

  • add a test that strings work (so it does not break again in the future). Is this is "it used to explode" issue, it does not need to be an image test. Can you also add a test that if you pass norm as a string and vmin and vmax that the norm object is what you expect and the vmin/vmax are set to what was passed in (the failure mode it is trying to catch would be the auto-scaling logic moving much earlier and getting in before we check "are you autoscaled")?
  • squash your commits into 1 to remove the added/deleted file from the history (we had an incident where someone committed a whole venv and then removed it in the history which was annoying to remove) see https://matplotlib.org/devdocs/devel/development_workflow.html#rewrite-commit-history

@tacaswell tacaswell added this to the v3.10.0 milestone Apr 19, 2024
@clementgilli
Copy link
Author

I squashed as you said, but I don't really know where add this tests, as I'm a new contributor.
For the first test, I don't understand what you are expecting.
For the second, here an idea :

ax =hexbin(rand(10), rand(10), norm="log",vmin=2,vmax=5)
assert ax.norm.vmin == 2
assert ax.norm.vmin == 5

@tacaswell
Copy link
Member

That code (along with probably checking the type of ax.norm) is exactly what I was asking for.

Putting the new test near

matplotlib/lib/matplotlib/tests/test_axes.py

Lines 969 to 978 in 199c31f

def test_hexbin_bad_extents():
fig, ax = plt.subplots()
data = (np.arange(20) / 20).reshape((2, 10))
x, y = data
with pytest.raises(ValueError, match="In extent, xmax must be greater than xmin"):
ax.hexbin(x, y, extent=(1, 0, 0, 1))
with pytest.raises(ValueError, match="In extent, ymax must be greater than ymin"):
ax.hexbin(x, y, extent=(0, 1, 1, 0))
makes sense to me, call it something like test_hexbin_string_norm

@tacaswell
Copy link
Member

Content of the test looks good! However, the linter needs to be placated.

@clementgilli
Copy link
Author

Sorry for the wait, it should be good now.

@tacaswell
Copy link
Member

I'm going to merge over the broken docs (it is an install issue that I think we have fixed on main).

@tacaswell tacaswell merged commit ba7dbf3 into matplotlib:main Apr 29, 2024
40 of 42 checks passed
@tacaswell
Copy link
Member

Thank you for your work on this @clementgilli and congratulations on your first merged Matplotlib PR 🎉

I hope we hear from you again.

@QuLogic QuLogic mentioned this pull request Jun 17, 2024
Closed
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

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
First Time Contributors
Status: Waiting for author
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@clementgilli @tacaswell @timhoffm