Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Improvements and bugfixes for hexbin marginals#18875
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This is the code I used for testing importnumpyasnpfrommatplotlibimportpyplotaspltnp.random.seed(0)x=np.random.normal(1,1.5,1000)y=np.random.normal(1,1.5,1000)plt.figure()plt.hexbin(x,y,gridsize=7,marginals=True,reduce_C_function=np.sum,extent=(-2,5,-3,5))# extents are here in correct orderplt.grid()plt.show()plt.figure()plt.hexbin(x,y,gridsize=10,marginals=True,reduce_C_function=np.sum,extent=(-2,5,5,-3))# extents are here in wrong orderplt.grid()plt.show() |
Can you add tests for the new stuff? |
I can try. It's my first time contributing here, so I'm not sure what the tests should check. Can you give me some pointers? |
Tests for hexbin would likely go in |
ping for tests |
I'm sorry for leaving it in this state, but probably in a few weeks is the earliest I'll have time to learn how the test system works and add the tests. It might make more sense for someone else to take it over. |
I'll mark as orphaned if someone else wants to take it up. On the other hand, you are more than welcome to readopt your own baby. The tests are in |
I'm going to try and revive this and split into multiple PRs. I'll add links in the original description at the top where the individual issues are helpfully listed. |
Apart from the new error in the PR I opened, I think the rest of the issues were fixed (possibly by#21039?) at some point between this PR being opened and now, so I'll close this PR. Feel free to open new issues about the hexbin marginals if I've got that wrong! |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
The marginals functionality for hexbin had a couple of hidden bugs and issues:
hexbinwith a tuple for thegridsizeresulted in an error.extentsin the wrong order. This did not raise an error, but broke the marginals. (Error on bad input to hexbin extents #27607)reduce_C_functioneven whenCwas not provided. With the defaultreduce_C_function, this meant applyingnp.meanto a set of ones, resulting in only binary values in the marginal.This PR fixes these issues. It makes marginals work with tuple
gridsize, raises aValueErrorwhen theextentsare in the wrong order, ignores points outside the extents, aligns the bins of the marginals to the hexagonal bins and uses summation whenCis not provided.In addition I changed the formatting of the code for the y marginal to match that of the x marginal so that it's easier to compare (and diff) them. You could consider somehow merging the two with a function, since most of the code is actually quite repetitive.
What would really be useful would be some unit tests for the hexbin marginals, but maybe someone else can help with that?
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).