Skip to content
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

Refactor hexbin(). #21352

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Refactor hexbin(). #21352

merged 1 commit into from
Feb 1, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 14, 2021

  • Avoid having to copy() x and y, by not overwriting the original
    x and y variables but instead storing the transformed values in
    trfx/trfy.
  • Directly construct lattice1 and lattice2 as flat arrays (they are
    flattened at the end anyways), which allows using flat indices:
    the C is None case, becomes a simple bincount, the C is not None
    case can use a list-of-lists instead of an object array.
  • Factor out the x/y marginals handling into a for-loop, which
    additionally allows inlining coarse_bin.
  • Make the factor of 2 between nx and ny clearer (in the for zname...
    loop setup). (See also [Bug]: Hexbin gridsize interpreted differently for x and y #21349 (comment).)
  • Construct marginals verts in a vectorized fashion.

Even if we decide to ultimately deprecate marginals, this should help with converting them to a reusable examples entry.

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • 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).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak
Copy link
Member

jklymak commented Oct 14, 2021

Are you claiming the factor of two is expected? This PR doesn't seem to change anything....

@anntzer
Copy link
Contributor Author

anntzer commented Oct 14, 2021

I'm not claiming it is expected (my quick guess is that it isn't), I'm just saying that the current implementation has nbins = nx in one place and nbins = 2 * ny in another, and this refactor shows that more clearly.

@anntzer anntzer force-pushed the hexbin branch 2 times, most recently from 5441bf6 to 50d6db0 Compare October 14, 2021 10:17
@timhoffm
Copy link
Member

timhoffm commented Oct 14, 2021

Avoid having to copy() x and y, by not overwriting the original
x and y variables but instead storing the transformed values in
trfx/trfy.

I find the names trfx/trfy not quite helpful. They are longish compared to x/y but still don't read well. Alternatives:

  • Use tx / ty or x_/y_ as minimal extensions to clarify that it's not x/y itself.
  • Do the copy on demand if you need to log the values.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 14, 2021

tx/ty it is.

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.

This only a partial review. There's possibly more, but I didn't have the time to dig into the algorithm yet.

lib/matplotlib/axes/_axes.py Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
- Avoid having to copy() `x` and `y`, by not overwriting the original
  `x` and `y` variables but instead storing the transformed values in
  `trfx`/`trfy`.
- Directly construct lattice1 and lattice2 as flat arrays (they are
  flattened at the end anyways), which allows using flat indices:
  the `C is None` case, becomes a simple `bincount`, the `C is not None`
  case can use a list-of-lists instead of an object array.
- Factor out the x/y marginals handling into a for-loop, which
  additionally allows inlining coarse_bin.
- Make the factor of 2 between nx and ny clearer (in the `for zname...`
  loop setup).
- Construct marginals `verts` in a vectorized fashion.
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.

I'm not sure we needed this refactor - I'd have preferred something that made the whole thing clearer, but...

@jklymak jklymak merged commit 3331777 into matplotlib:main Feb 1, 2022
@anntzer anntzer deleted the hexbin branch February 1, 2022 12:03
@QuLogic QuLogic added this to the v3.6.0 milestone Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants