Added scalloped path effect #1609

nawendt · 2020-12-14T22:58:43Z

Description Of Changes

This commit adds a path effect to make lines drawn appear to be scalloped. This was based off of modifying the matplotlib TickedStroke path effect. This style of line is often used for highlighting areas in SPC MCD products.

Here is an example of what it looks like:

It seemed to make the most sense in the plot._mpl module, but that can be changed if needed.

Checklist

Tests added
Fully documented

dopplershift · 2020-12-16T02:40:09Z

This is super cool, thanks for submitting--I'm excited you got this working so relatively easily. I haven't done a detailed review, but a couple things jump out:

Can you add an image test for this? We have a section in the Contributor's Guide, but we may need more information there.
Instead of adding to plots/_mpl.py (which is only there to hold things that we borrow or are backporting for matplotlib), how about a new file: plots/patheffects.py? Then we can put in the hooks to pull the class up into the metpy.plots namespace (so we can rename patheffects.py if we need to in the future).

lgtm-com · 2020-12-16T06:53:29Z

This pull request introduces 1 alert when merging 787766d into b5e3942 - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

lgtm-com · 2020-12-16T14:30:36Z

This pull request introduces 1 alert when merging 96328fd into b5e3942 - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

nawendt · 2020-12-16T14:43:38Z

Failing check is due to old version of matplotlib that does not have patheffects._subclass_with_normal method. Do we need to patch that in for older versions?

dopplershift · 2020-12-16T17:42:04Z

If that came in with the recent matplotlib PR and won't appear until 3.4, then yeah. That part would go in _mpl.py.

lgtm-com · 2020-12-16T21:25:57Z

This pull request introduces 1 alert when merging b7b5337 into db732bd - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

lgtm-com · 2020-12-16T23:15:43Z

This pull request introduces 1 alert when merging 87c3a7d into db732bd - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

lgtm-com · 2020-12-17T14:21:43Z

This pull request introduces 1 alert when merging 823a808 into db732bd - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

This commit adds a path effect to make lines drawn appear to be scalloped. This was based off of modifying the matplotlib TickedStroke path effect. This style of line is often used for highlighting areas in SPC MCD products. Places it in metpy.plots.patheffects module with patches for versions of matplotlib < 3.2 in the metpy.plots._mpl module.

lgtm-com · 2020-12-17T15:06:50Z

This pull request introduces 1 alert when merging f9dac66 into db732bd - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

nawendt · 2020-12-17T15:18:08Z

I think everything is in order now. I cannot figure out why the Build Docs (3.9) fails. The output does not indicate its anything with the additions.

nawendt · 2020-12-22T15:38:22Z

In some of my testing with this on plotted maps, I've noticed that the same parameters given to the scallops will look different depending on the the selected area/extent. It seems to be related to the aspect ratio of the area selected. I find that when I manually multiply the scallop parameters by the data coordinates aspect ratio that it looks the same between varying areas. Setting the aspect to equal in matplotlib did not fix the issue. Is there a way to do this when the scallops get created instead of doing it manually? I looked into it some but could not find an obvious way to get it at a lower level.

nawendt · 2020-12-23T06:13:39Z

Well, it seems I may not have been setting the aspect ratio correctly. Using Axes.axis(equal) ended up changing data limits. Using Axes.set_aspect(1) did work as expected. Now things look fairly similar without manual scaling. May not make too much difference either ways as there are subtle differences no matter what method I use. I've been staring at these too long...

lgtm-com · 2021-02-22T22:52:03Z

This pull request introduces 1 alert when merging f9dac66 into 1aaf805 - view on LGTM.com

new alerts:

1 for 'import *' may pollute namespace

dopplershift

sigh Well I see I never clicked 'submit' on some of my last review comments. Any interest in reviving this @nawendt? I think we can just depend on Matplotlib 3.4 at this point, otherwise there were just a couple minor things I found last time I looked at this.

Sorry, I thought this was waiting on your end, not mine.

dopplershift · 2021-06-23T22:19:03Z

tests/plots/test_patheffects.py

+import pytest
+
+from metpy.plots import patheffects
+# Fixtures to make sure we have the right backend and consistent round


Suggested change

# Fixtures to make sure we have the right backend and consistent round

# Fixture to make sure we have the right backend

dopplershift · 2021-06-23T22:19:27Z

tests/plots/test_patheffects.py

@@ -0,0 +1,44 @@
+# Copyright (c) 2018 MetPy Developers.


Suggested change

# Copyright (c) 2018 MetPy Developers.

# Copyright (c) 2020 MetPy Developers.

nawendt · 2023-03-29T19:01:55Z

Yes. I was actually looking at this recently. I know I talked about trying to get them to look the same for different spatial extents. At least with looking at these on a map, the pixels will represent different sizes depending on the extent. If you get the average meters per pixel (between x and y directions), you can use that to scale the sizing parameters to have some static "scallops per pixel" so to speak. In NMAP they do not try and keep things static based on extent. So that's the remaining question I have: do we do anything to account for such things or should the user be responsible for getting to to look "right" for their use case?

dopplershift · 2023-03-29T19:37:39Z

Ideally, I think we account for those effects to make everything work as perfectly as possible out of the box. However, those little details can make the job a LOT harder to get to completion, and we don't want to hold up something useful just for that. So I'm willing to merge and if you or someone else is interested in getting the last part "right", we can do that later.

"Don't let perfect be the enemy of good" and all that. (I'm about to talk about something similar over on #2420)

akrherz · 2023-03-29T19:52:45Z

src/metpy/plots/patheffects.py

+                                 [np.sin(theta), np.cos(theta)]])
+
+        # Convert spacing parameter to pixels.
+        spacing_px = renderer.points_to_pixels(self._spacing)


Does this "blow up" when using a PS/SVG backend that does not have the concept of pixels?

It's part of the core RendererBase interface. In that case it's basically a noop (returns points).

See https://github.com/matplotlib/matplotlib/blob/c5abf906043c296ca0752ad5873b635c91939226/lib/matplotlib/backend_bases.py#L689-L707

Thanks and sorry, I was vague. I was wondering if passing a points value of say 30 would cause the rendered path on SVG to look wildly bad. I've personally struggled with an issue like this attempting to figure out how something will look within matplotlib figure before the renderer is called/known.

I haven't tried it on this PR, but it's working great on #2420 with the same call.

OK, thank you! This all is very cool and thanks @nawendt !

nawendt · 2023-04-05T20:58:45Z

Ok. I have something worked up. I just pulled from this PR since I cannot find my old local copy. I pushed those commits to my fork with the same name. However, it looks like GitHub does not want to link those commits to this PR. I probably worked on this on a computer I no longer use back in the day. Can I do anything about connecting things here or do I have to open a new PR?

Also, it looks to me like the all the scaling works as expected. Everything is done in pixels. Things can look a bit odd when zoomed out, but, again, I think matplotlib is doing what it is supposed to. I have attached an example imaged of a scalloped area zoomed out (the settings are the same as the formal test). If I were to try and normalize things, I think it just would look like very small scallops and just look bad for another reason. Anyway, you let me know if we need some more work on the scaling.

dopplershift · 2023-04-05T21:55:00Z

Weird on the GitHub problems. If nothing immediate jumps out, it's fine to just open a new PR and not worth spending value dev minutes debugging it.

As far as the scaling goes, what you have above looks great to me and worth including as-is. Again we can always revise in-tree if someone discovers some better matplotlib-fu that makes things look even better. The main questions are always:

Does this work correctly
Does it add things that are going to make people happy to use it and make their lives easier
Is it written in a way that we can keep this working in the future

Note that "is perfect" appears nowhere in that list. 😉

dopplershift · 2023-04-05T21:56:20Z

I'll also add that I suspect you are going to be using this the most of anyone, so if it's good for your purposes, I think the rest of us will be more than happy.

nawendt · 2023-04-05T23:14:58Z

Cannot recover branch this links to. Closed and continued in #2992.

nawendt requested review from dcamron and dopplershift as code owners December 14, 2020 22:58

dopplershift added this to the 1.1.0 milestone Jan 7, 2021

Base automatically changed from master to main February 22, 2021 22:39

dopplershift modified the milestones: 1.1.0, 1.2.0 Aug 2, 2021

dopplershift added Type: Feature New functionality Area: Plots Pertains to producing plots labels Nov 5, 2021

dopplershift modified the milestones: 1.2.0, 1.3.0 Nov 29, 2021

dopplershift modified the milestones: 1.3.0, May 2022 Mar 31, 2022

dopplershift modified the milestones: May 2022, July 2022 May 16, 2022

dopplershift removed this from the October 2022 milestone Oct 18, 2022

dopplershift reviewed Mar 29, 2023

View reviewed changes

akrherz reviewed Mar 29, 2023

View reviewed changes

nawendt mentioned this pull request Apr 5, 2023

Add scalloped path effect #2992

Merged

2 tasks

nawendt closed this Apr 5, 2023

	# Fixtures to make sure we have the right backend and consistent round
	# Fixture to make sure we have the right backend

	# Copyright (c) 2018 MetPy Developers.
	# Copyright (c) 2020 MetPy Developers.

Added scalloped path effect #1609

Added scalloped path effect #1609

Uh oh!

Conversation

nawendt commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description Of Changes

Checklist

Uh oh!

dopplershift commented Dec 16, 2020

Uh oh!

lgtm-com bot commented Dec 16, 2020

Uh oh!

lgtm-com bot commented Dec 16, 2020

Uh oh!

nawendt commented Dec 16, 2020

Uh oh!

dopplershift commented Dec 16, 2020

Uh oh!

lgtm-com bot commented Dec 16, 2020

Uh oh!

lgtm-com bot commented Dec 16, 2020

Uh oh!

lgtm-com bot commented Dec 17, 2020

Uh oh!

lgtm-com bot commented Dec 17, 2020

Uh oh!

nawendt commented Dec 17, 2020

Uh oh!

nawendt commented Dec 22, 2020

Uh oh!

nawendt commented Dec 23, 2020

Uh oh!

lgtm-com bot commented Feb 22, 2021

Uh oh!

dopplershift left a comment

Choose a reason for hiding this comment

Uh oh!

dopplershift Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

dopplershift Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

nawendt commented Mar 29, 2023

Uh oh!

dopplershift commented Mar 29, 2023

Uh oh!

akrherz Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

dopplershift Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

akrherz Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

dopplershift Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

akrherz Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

nawendt commented Apr 5, 2023

Uh oh!

dopplershift commented Apr 5, 2023

Uh oh!

dopplershift commented Apr 5, 2023

Uh oh!

nawendt commented Apr 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nawendt commented Dec 14, 2020 •

edited

Loading