Skip to content

Add feature: adapt_limit argument #26381

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

Closed

Conversation

niranjank2022
Copy link
Contributor

PR summary

PR checklist

This PR is to address issue #26288.

@tacaswell
Copy link
Member

Thank you for your work on this, however I am not sure that this is the right approach. We have a system for managing the margins / autolimiting behivor (see https://matplotlib.org/stable/gallery/subplots_axes_and_figures/axes_margins.html#sphx-glr-gallery-subplots-axes-and-figures-axes-margins-py). The best option would be integrate hist2d with this system (maybe just dropping the set_xlim and set_ylim calls!) or include a clear explanation as to what the limitations with margins are and why it can not be made to work without an additional parameter to just hist2D.

@tacaswell
Copy link
Member

@niranjank2022 Reading this a bit more carefully and understanding the original bug, I do not think that this is a feature we want to take.

Thank you for your effort and I hope we hear from you again!

@tacaswell tacaswell closed this Jul 27, 2023
@Hvass-Labs
Copy link

@niranjank2022 Thanks very much for opening this PR!

I have had a lengthy debate with @tacaswell whether to include the feature and he is very difficult to persuade :-)

I can see you are a young student who wants to contribute to open source, so I thought I would give a few (hopefully) helpful comments as a thanks for your help.

  1. If you are essentially just submitting someone else's code it is always a good idea to give a little acknowledgement of the source and original author in your PR comment. I won't get mad but others might.

  2. My original code was beautifully commented so anyone can easily understand what it is doing. All my own code both private and public is written like that, and it means I can manage fairly large and complex code-bases, and very quickly understand what my old code is doing, because I just read the English comments and don't have to "decipher" the code first. I browsed through Matplotlib's style-guide just now, and it doesn't seem to mention how to write inline code-comments. So in this case I think you should just have left my comments in the code, and the core developers might have complained if they thought it was too much.

  3. The padding constants should probably be omitted, and then people would have to pad the axis limits themselves.

  4. Some of the variables should perhaps be renamed to match the Matplotlib naming style. For example, x_min to xmin, and mask to h_mask (I would keep the underscore here), and mask_flat_x to x_mask_flat.

Hopefully the core devs will help you finish this PR, and you will be encouraged to contribute more code in the future!

@niranjank2022
Copy link
Contributor Author

@niranjank2022 Thanks very much for opening this PR!

I have had a lengthy debate with @tacaswell whether to include the feature and he is very difficult to persuade :-)

I can see you are a young student who wants to contribute to open source, so I thought I would give a few (hopefully) helpful comments as a thanks for your help.

  1. If you are essentially just submitting someone else's code it is always a good idea to give a little acknowledgement of the source and original author in your PR comment. I won't get mad but others might.
  2. My original code was beautifully commented so anyone can easily understand what it is doing. All my own code both private and public is written like that, and it means I can manage fairly large and complex code-bases, and very quickly understand what my old code is doing, because I just read the English comments and don't have to "decipher" the code first. I browsed through Matplotlib's style-guide just now, and it doesn't seem to mention how to write inline code-comments. So in this case I think you should just have left my comments in the code, and the core developers might have complained if they thought it was too much.
  3. The padding constants should probably be omitted, and then people would have to pad the axis limits themselves.
  4. Some of the variables should perhaps be renamed to match the Matplotlib naming style. For example, x_min to xmin, and mask to h_mask (I would keep the underscore here), and mask_flat_x to x_mask_flat.

Hopefully the core devs will help you finish this PR, and you will be encouraged to contribute more code in the future!

Thanks @Hvass-Labs for your valuable comments. It means a lot to me. I will definitely keep these in mind and continue to find ways to contribute! :)

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.

[ENH]: Adapt 2-dim histogram axis limits to content
4 participants