8000 Added linear scaling test to Hexbin marginals by j-bowhay · Pull Request #21339 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Added linear scaling test to Hexbin marginals #21339

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

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

j-bowhay
Copy link
Contributor

PR Summary

Added test for hexbin marginals that uses a linear scale. Closes #21165

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [N/A] New features are documented, with examples if plot related.
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@j-bowhay j-bowhay changed the title Test hexbin linear Added linear scaling test to Hexbin marginals Oct 11, 2021
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 while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.

Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Seems good, but snapping should be default and you will need to change the image.

@j-bowhay
Copy link
Contributor Author

Seems good, but snapping should be default and you will need to change the image.

Have changed snapping to default and updated the image

Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Sorry, one more request - we try and reduce the number of tests with fonts in them (when we un-pin font tools it changes a bunch of images). I know the other test has fonts, but can this one not have them? i.e. remove_text=True in the test parameters?

@jklymak jklymak requested a review from dstansby October 13, 2021 07:48
@jklymak
Copy link
Member
jklymak commented Oct 13, 2021

If anyone merges this, please squash merge (or @j-bowhay you can rebase and squash the commits, and force push back here (make a back up branch first if you aren't used to doing this!)). This is particularly important to prevent the stray image from being in the repo. Thanks for your patience with the process!.

Copy link
Member
@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I block this because there is a bug with the gridsize parameter #21349 and we want that fixed before adding a reference image.

Anybody can dismiss after #21349 is fixed and a new reference image is push (ideally with a squash) - Otherwise, whoever discards this should squash-merge the PR.

@jklymak
Copy link
Member
jklymak commented Oct 14, 2021

lets move to draft until we sort out. @j-bowhay please ping us if the issue with hex bin does not get resolved soon.

@jklymak jklymak marked this pull request as draft October 14, 2021 07:49
@tacaswell
Copy link
Member

This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream"

git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease   # assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

@QuLogic QuLogic mentioned this pull request Nov 12, 2021
7 tasks
Copy link
Member
@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

This definitely needs updating or adopting in it's current state - see the above comment!

@llricci
Copy link
Contributor
llricci commented Jun 11, 2022

Hey @j-bowhay, are you still working in this PR? Because it is been inactive for a long time. Hope I am not disturbing you.

@j-bowhay
Copy link
Contributor Author

Hi @LucasRicci this was blocked on #21349 by @timhoffm so I have not made the requested changes yet. I see there has been no progress on this issue yet. I am happy to finish the pr once the issue is fixed or if it is decided to add these test with the bug present and then update later.

@llricci
Copy link
Contributor
llricci commented Jun 11, 2022

@j-bowhay Oh I see, thanks

@j-bowhay j-bowhay force-pushed the test-hexbin-linear branch from 45e04ed to 1d12b60 Compare July 7, 2022 21:05
@j-bowhay j-bowhay marked this pull request as ready for review July 7, 2022 21:05
@j-bowhay
Copy link
Contributor Author
j-bowhay commented Jul 7, 2022

@timhoffm I am correct in thinking this is no longer blocked by #21349? I have sorted out the git stuff and resolved the comments

@j-bowhay j-bowhay requested review from timhoffm and dstansby July 8, 2022 15:56
@timhoffm timhoffm dismissed stale reviews from dstansby and themself July 12, 2022 21:47

Not blocked by #21349 anymore because, that turned out to be not a bug.

@timhoffm timhoffm merged commit 8e177ba into matplotlib:main Jul 12, 2022
@timhoffm
Copy link
Member

@j-bowhay thanks for the ping, and thanks for the PR!

@QuLogic QuLogic added this to the v3.6.0 milestone Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hexbin marginals need a test for linear scaling
7 participants
0