Refactor hexbin(). - #21352
Refactor hexbin().#21352
Conversation
|
I'm not claiming it is expected (my quick guess is that it isn't), I'm just saying that the current implementation has |
5441bf6 to
50d6db0
Compare
I find the names
|
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
This only a partial review. There's possibly more, but I didn't have the time to dig into the algorithm yet.
- 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.
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
I'm not sure we needed this refactor - I'd have preferred something that made the whole thing clearer, but...
4 participants
xandy, by not overwriting the originalxandyvariables but instead storing the transformed values intrfx/trfy.flattened at the end anyways), which allows using flat indices:
the
C is Nonecase, becomes a simplebincount, theC is not Nonecase can use a list-of-lists instead of an object array.
additionally allows inlining coarse_bin.
for zname...loop setup). (See also [Bug]: Hexbin gridsize interpreted differently for x and y #21349 (comment).)
vertsin 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
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).