Skip to content

Add example to histogram colorbar on galleries #30107

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 31 commits into from
Jun 8, 2025

Conversation

livlutz
Copy link
Contributor

@livlutz livlutz commented May 25, 2025

PR summary

This PR contains an example code for generating histogram colorbar on the galleries section with a reference to it on the readme file
closes #30026

PR checklist

@github-actions github-actions bot added the Documentation: examples files in galleries/examples label May 25, 2025
@livlutz
Copy link
Contributor Author

livlutz commented May 25, 2025

@story645 thanks for the suggestion! This PR is ready for review

@rcomer
Copy link
Member

rcomer commented May 25, 2025

It looks like something went wrong with the layout. Also, please read the comments on the issue for ideas how to improve this.

@livlutz
Copy link
Contributor Author

livlutz commented May 25, 2025

thanks for the feedback, I tried to make some adjustments but I cannot see if my changes were successful, how does this link you commented before work?

@rcomer
Copy link
Member

rcomer commented May 25, 2025

how does this link you commented before work?

At the bottom of this page are some automated checks. Scroll to the bottom of them and you see "View the built docs". Click on that and you can then navigate to your example from there.

But you can also just run your script locally to see how the plot looks. That will give you faster feedback.

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

As @rcomer said, please look at the original issue - particularly the suggestion to use binomially distributed data .

@livlutz
Copy link
Contributor Author

livlutz commented May 26, 2025

Thanks a lot for all the suggestions, I tried to see the changes locally like @rcomer said and it should look much better now, please let me know if there's still something missing

@story645
Copy link
Member

story645 commented May 26, 2025

Can you please try the dataset @jklymak suggested in the original issue?

Also can you remove the spines (code should also be in the original example). Also please remove the yaxis_inversion as that's confusing me here - it might be clearer if you add the colorbar like in the original issue. Which also setting the margins to 0 (see original example) will remove the spacing for the bar chart so that it takes up the whole space (also please set height to 100% so they line up) - you are welcome to add that as a comment if it makes it clearer and to ask about the purpose of any code you don't understand.

@livlutz
Copy link
Contributor Author

livlutz commented May 26, 2025

I tried the first example from here : https://matplotlib.org/devdocs/gallery/images_contours_and_fields/image_demo.html and it worked just fine! please tell me if it is alright now

@story645
Copy link
Member

story645 commented May 26, 2025

The new dataset isn't a normal distribution so the histogram should also not be a normal distribution. Please check your bins and extents (for example the dark blue and dark orange should be more prominent).

sphx_glr_colorbar_histogram_001_2_00x.png

Please plot the histogram without preset bins so that you can see what it's supposed to look like and then follow the example in the original issue for visualizing the correct histogram. Please ask questions if you don't understand the code.

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

Also please remove the spines and margins from the histogram.

@livlutz
Copy link
Contributor Author

livlutz commented May 26, 2025

The new dataset isn't a normal distribution so the histogram should also not be a normal distribution. Please check your bins and extents (for example the dark blue and dark orange should be more prominent).

sphx_glr_colorbar_histogram_001_2_00x.png

Please plot the histogram without preset bins so that you can see what it's supposed to look like and then follow the example in the original issue for visualizing the correct histogram. Please ask questions if you don't understand the code.

I took a look at the original issue but it says the new dataset is a bivariate normal distribution, should I still change the histogram? And I'm a bit confused about the extents, do you have any suggestions on what changes should I make?

…, improve inset axes layout, and enhance label spacing
@livlutz
Copy link
Contributor Author

livlutz commented May 26, 2025

I changed the histogram slightly so it depicts a bivariate normal distribuition, please tell me if I still need to fix anything

@livlutz
Copy link
Contributor Author

livlutz commented May 26, 2025

Yes and it looks slightly different from the output I got when I ran the program locally...does it have to do with the output screen scale?

@rcomer
Copy link
Member

rcomer commented Jun 6, 2025

The mypy problems have been fixed on the main branch. It might be enough to close and re-open the PR to pick up those changes.

@rcomer rcomer closed this Jun 6, 2025
@github-project-automation github-project-automation bot moved this from Needs review to Waiting for author in First Time Contributors Jun 6, 2025
@rcomer rcomer reopened this Jun 6, 2025
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.

Overall, the commenting feels a bit too noisy. Please recheck with our example guidelines and see if you can improve a bit.

I have the impression that comments are used to create sections with headings (# === Surface Data ===). IMHO that's too much for examples. The code should mostly be able to stand for itself. If comments are used, they should be an explantion, i.e. you'd typically write # surface data but not # === Surface Data ===.

@livlutz
Copy link
Contributor Author

livlutz commented Jun 6, 2025

Thanks for the help! I managed to get rid of linting errors (finally!) so this is ready for review.

@livlutz
Copy link
Contributor Author

livlutz commented Jun 7, 2025

I applied the suggested changes and I also checked how it looks like on the built docs and everything looks alright, do we need to change anything else?

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.

Gerneally, please lowercase the comments. As said, they are not section titles but remarks.

@livlutz
Copy link
Contributor Author

livlutz commented Jun 7, 2025

I'm just getting an error in line 30 : line 30, in
norm = mcolors.BoundaryNorm(bins, cmap.N)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/lib/python3.12/site-packages/matplotlib/colors.py", line 2042, in init
super().init(vmin=boundaries[0], vmax=boundaries[-1], clip=clip)
~~~~~~~~~~^^^
TypeError: 'int' object is not subscriptable

I am not really sure what it means and I think we should change the parameters

@livlutz
Copy link
Contributor Author

livlutz commented Jun 7, 2025

Thank you for the help, I managed to fix it now! Hopefully it works alright now

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.

Only minor formatting comments left.

cax = ax.inset_axes([1.18, 0.02, 0.25, 0.95]) # left, bottom, width, height

# plot histogram
counts, bin_edges = np.histogram(Z, bins=bins)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
counts, bin_edges = np.histogram(Z, bins=bins)
counts, _ = np.histogram(Z, bins=bin_edges)

Sorry, I just realized, we can make this now slightly safer/more obvious by using the existing bin_edges and not let np.histogram create them anew, hoping they are identical to the explicit calculation above.

@livlutz
Copy link
Contributor Author

livlutz commented Jun 8, 2025

Is there anything else left to change?

@timhoffm timhoffm added this to the v3.11.0 milestone Jun 8, 2025
@timhoffm timhoffm merged commit 3574a7e into matplotlib:main Jun 8, 2025
22 checks passed
@timhoffm
Copy link
Member

timhoffm commented Jun 8, 2025

Thanks @livlutz and congratulations on your first contribution to matplotlib! 🎉 We hope to see you again.

@livlutz
Copy link
Contributor Author

livlutz commented Jun 8, 2025

Thanks @livlutz and congratulations on your first contribution to matplotlib! 🎉 We hope to see you again.

Thank you all for your help! Hope to be able to contribute again soon :)

@QuLogic QuLogic moved this from Waiting for author to Merged in First Time Contributors Jun 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: examples files in galleries/examples
Projects
Development

Successfully merging this pull request may close these issues.

[Doc]: add histogram as colorbar example
5 participants