Skip to content
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

Merged

Conversation

gautamsagar99
Copy link
Contributor

@gautamsagar99 gautamsagar99 commented Aug 26, 2023

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

Copy link

@github-actions github-actions bot left a 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
Copy link
Contributor

oscargus commented Aug 27, 2023

It does not produce identical results. Compare the second plot where the "innermost" part of the top-right contour is no longer yellow.

With that said, I do not know if it should be there, etc, or if one should add an example to show the difference with using the origin keyword.

@rcomer
Copy link
Member

rcomer commented Aug 27, 2023

@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”.

@oscargus
Copy link
Contributor

Ahh, sorry I mixed up the versions (checked on my phone)...

@story645
Copy link
Member

I also wondered if it worth adding an example to show use of origin

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?

@gautamsagar99
Copy link
Contributor Author

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
import numpy as np

x = np.arange(1, 10)
y = x.reshape(-1, 1)
h = x * y

cs = plt.contourf(h, levels=[10, 30, 50],
colors=['#808080', '#A0A0A0', '#C0C0C0'], extend='both', origin="upper")
cs.cmap.set_over('red')
cs.cmap.set_under('blue')
cs.changed()

@story645
Copy link
Member

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?

@gautamsagar99
Copy link
Contributor Author

@rcomer Let me add a new section below which clearly populates the example using origin keyword.

@story645 Yes we can add side by side which will be clearly differentiating for lower and upper origin value.

Let me Just add it and push it.

Thanks

@gautamsagar99
Copy link
Contributor Author

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 origin parameter in Matplotlib's contourf function to adjust the positioning of grid data. The origin parameter specifies whether data coordinates originate from the upper left ('upper') or lower left ('lower') corner of the grid.

import matplotlib.pyplot as plt
import numpy as np

x = np.arange(1, 10)
y = x.reshape(-1, 1)
h = x * y

fig, (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()

plt.show()

This code creates two side-by-side contour plots with different origin settings ('upper' and 'lower'). The contourf function utilizes the origin parameter to control how grid data coordinates are interpreted for plotting. The resulting visualizations highlight the effect of changing the origin on the contour plot's appearance."

@story645
Copy link
Member

@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.

@gautamsagar99
Copy link
Contributor Author

@story645 I will keep this as heading "Customizing Contour Plots with the origin Keyword:", under it I will provide the example.

@gautamsagar99
Copy link
Contributor Author

@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?

@story645
Copy link
Member

Auto generate - see https://matplotlib.org/devdocs/devel/document.html#write-examples-and-tutorials

@gautamsagar99
Copy link
Contributor Author

@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
# ------------------
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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,)
)
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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.

@gautamsagar99
Copy link
Contributor Author

@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.

Comment on lines 116 to 125
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()
Copy link
Member

@timhoffm timhoffm Aug 30, 2023

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:

Suggested change
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")

grafik

Copy link
Contributor Author

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.

@story645 story645 added this to the v3.8-doc milestone Aug 30, 2023
@story645 story645 added the Documentation: examples files in galleries/examples label Aug 30, 2023
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.

title case is a style issue that needs to be amended, the other suggestion is more for clarity,

Comment on lines 107 to 110
# Customizing Contour Plots with the origin Keyword
# -------------------------------------------------
# This code showcases contour plot customization using the origin keyword,
# demonstrating distinct upper and lower origin perspectives.
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
# 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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

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.

Thanks for all your work! Would you like me to squash merge or do you want to try rebasing?

Comment on lines 107 to 108
# Orient contour plots using the origin keyword
# -----------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Should match:

Suggested change
# Orient contour plots using the origin keyword
# -----------------------------------------------
# Orient contour plots using the origin keyword
# ---------------------------------------------

@gautamsagar99
Copy link
Contributor Author

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!

@story645 story645 linked an issue Aug 31, 2023 that may be closed by this pull request
@story645
Copy link
Member

Thanks! You need to make the change @QuLogic suggested, and also much credit to @rcomer for opening the issue & her and @oscargus for clarifying.

@gautamsagar99
Copy link
Contributor Author

Sure, Let me make that small change and push it.

@timhoffm timhoffm merged commit d3afa55 into matplotlib:main Aug 31, 2023
19 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Aug 31, 2023
@QuLogic QuLogic modified the milestones: v3.8-doc, v3.8.0 Aug 31, 2023
QuLogic added a commit that referenced this pull request Aug 31, 2023
…608-on-v3.8.x

Backport PR #26608 on branch v3.8.x (Removed unnecessary origin keywords)
@story645
Copy link
Member

congrats on your first merged pr 🎊 hope to see you back for more!

@gautamsagar99
Copy link
Contributor Author

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! 🙌😊

@ksunden ksunden mentioned this pull request Sep 15, 2023
5 tasks
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]: contourf demo use of origin keyword
6 participants