-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Removed unnecessary origin keywords #26608
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
Removed unnecessary origin keywords #26608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
@oscargus which versions are you comparing? There seems to have been a bug in recent Matplotlib versions (but not main or the release candidate) that caused that yellow not to appear. I already confused myself with that a couple of days ago: https://discourse.matplotlib.org/t/cmap-set-over-and-set-under-not-working-in-newer-python-environments/23999 I also wondered if it worth adding an example to show use of origin, and I see @gautamsagar99 asked the same on the issue (#26600). I know there is also a tension with “how much is too much to put in an example”. |
Ahh, sorry I mixed up the versions (checked on my phone)... |
I think here it'd be useful to clarify what sounds like a point of confusion. I'd also make it a standalone "origin" section of an existing example? |
Hey, To clarify I am using matplotlib - 3.7.1 version and I used colab for testing all the examples. Here is the link for colab: https://colab.research.google.com/drive/1XnsYLa8Tr0zuLe4hRDRu4KBJcJKlEnXX?usp=sharing If you want me to add a new section with the below example I can add it. import matplotlib.pyplot as plt x = np.arange(1, 10) cs = plt.contourf(h, levels=[10, 30, 50], |
building on your example, I think a side by side might be clearest: x = np.arange(1, 10)
y = x.reshape(-1, 1)
h = x * y
fog, (ax1, ax2) = plt.subplots(ncols=2)
for (ax, origin) in [(ax1, 'upper'), (ax2, 'lower')]:
ax.set_title(f"origin={origin}")
cs = ax.contourf(h, levels=[10, 30, 50],
colors=['#808080', '#A0A0A0', '#C0C0C0'], extend='both', origin=origin)
cs.cmap.set_over('red')
cs.cmap.set_under('blue')
cs.changed() and I'm a big fan here of using sphinx headings for each of the examples 'cause they all demonstrate a different feature? |
Finally I want to add The below section to the documentation: "**Customizing Contour Plots with the origin Keyword: ** This section demonstrates how to use the
This code creates two side-by-side contour plots with different |
@gautamsagar99 seems good to me. Since you're modifying this example, can you also put in headings id'ing the topics of the other examples? I also understand if not because it's out of scope here. |
@story645 I will keep this as heading "Customizing Contour Plots with the origin Keyword:", under it I will provide the example. |
@story645 I'm Confused regarding the images. Do we need to upload the image for the output(for the above example) or It is going to auto generate? |
Auto generate - see https://matplotlib.org/devdocs/devel/document.html#write-examples-and-tutorials |
@story645 Thank you for your assistance! I've made the necessary changes and have also generated the documentation. Please review the updates and let me know if any further adjustments are required. |
fig.colorbar(cs, ax=ax, shrink=0.9) | ||
ax.set_title("extend = %s" % extend) | ||
ax.locator_params(nbins=4) | ||
|
||
plt.show() | ||
|
||
# %% | ||
# Customizing Contour Plots with the origin Keyword | ||
# ------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc build is failing because this needs to go the whole length of the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will Adjust this in small sentence like "Contour Plots using origin Keyword" and modify similarly based on the previous titles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not about the length of the title, it's that ---------
needs to be the width of the title it's under
linewidths=(3,), | ||
origin=origin) | ||
linewidths=(3,) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is wrapped in the original but can probably be one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to wrap this in a single line or two
# %% | ||
# Customizing Contour Plots with the origin Keyword | ||
# ------------------ | ||
# This section demonstrates how to use the origin parameter in Contour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what information is this sentence adding in addition to the section title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I will think of nice sentence which makes more valuable and more precise in explaining the below code snippet.
@story645 I have done all the changes I hope everything looks good. Let me know if there is anything if you might want me to modify. |
fig, (ax1, ax2) = plt.subplots(ncols=2) | ||
# Note: lower and upper are the values provided | ||
# to origin argument in contourf. | ||
for (ax, origin) in [(ax1, 'upper'), (ax2, 'lower')]: | ||
ax.set_title(f"origin={origin}") | ||
cs = ax.contourf(h, levels=[10, 30, 50], colors=[ | ||
'#808080', '#A0A0A0', '#C0C0C0'], extend='both', origin=origin) | ||
cs.cmap.set_over('red') | ||
cs.cmap.set_under('blue') | ||
cs.changed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this to:
fig, (ax1, ax2) = plt.subplots(ncols=2) | |
# Note: lower and upper are the values provided | |
# to origin argument in contourf. | |
for (ax, origin) in [(ax1, 'upper'), (ax2, 'lower')]: | |
ax.set_title(f"origin={origin}") | |
cs = ax.contourf(h, levels=[10, 30, 50], colors=[ | |
'#808080', '#A0A0A0', '#C0C0C0'], extend='both', origin=origin) | |
cs.cmap.set_over('red') | |
cs.cmap.set_under('blue') | |
cs.changed() | |
fig, (ax1, ax2) = plt.subplots(ncols=2) | |
ax1.set_title("origin='upper'") | |
ax2.set_title("origin='lower'") | |
ax1.contourf(h, levels=np.arange(5, 70, 5), extend='both', origin="upper") | |
ax2.contourf(h, levels=np.arange(5, 70, 5), extend='both', origin="lower") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look more optimized. Thanks, Let me add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title case is a style issue that needs to be amended, the other suggestion is more for clarity,
# Customizing Contour Plots with the origin Keyword | ||
# ------------------------------------------------- | ||
# This code showcases contour plot customization using the origin keyword, | ||
# demonstrating distinct upper and lower origin perspectives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Customizing Contour Plots with the origin Keyword | |
# ------------------------------------------------- | |
# This code showcases contour plot customization using the origin keyword, | |
# demonstrating distinct upper and lower origin perspectives. | |
# Orient contour plots using the `origin` keyword | |
# ------------------------------------------------- | |
# This code demonstrates orienting contour plot data using the *origin* keyword. |
Our convention is Title case - first word in title upper case, everything else lower case.
Also origin isn't a customization , it's a tell Matplotlib my data needs to be read from the lower left or upper left corner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@story645 Thank you for sharing. I will update the changes and push it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi, changes aren't showing up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh is it, Let me recommit and push the latest changes again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work! Would you like me to squash merge or do you want to try rebasing?
# Orient contour plots using the origin keyword | ||
# ----------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should match:
# Orient contour plots using the origin keyword | |
# ----------------------------------------------- | |
# Orient contour plots using the origin keyword | |
# --------------------------------------------- |
Huge thanks to @story645 and @timhoffm I wanted to express my sincere appreciation for your tremendous support during my first open source contribution. Your guidance in understanding automatic documentations with proper formatting and your help in fixing my mistakes were incredibly valuable. Thanks for making this journey so enriching! |
Sure, Let me make that small change and push it. |
…608-on-v3.8.x Backport PR #26608 on branch v3.8.x (Removed unnecessary origin keywords)
congrats on your first merged pr 🎊 hope to see you back for more! |
Thank you so much for your guidance throughout the process! Contributing to this open-source project has been an amazing learning experience, and I'm really grateful for the opportunity. I completely understand that reviewing and merging PRs is no small task, and your support in guiding me through it means a lot. Looking forward to continuing this journey and contributing more in the future! 🙌😊 |
PR summary
Removed origin keyword as it is not required in the provided examples.
https://matplotlib.org/devdocs/gallery/images_contours_and_fields/contourf_demo.html