8000 Cleanup arrow_demo. by anntzer · Pull Request #20390 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Cleanup arrow_demo. #20390

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 1 commit into from
Jun 11, 2021
Merged

Cleanup arrow_demo. #20390

merged 1 commit into from
Jun 11, 2021

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented Jun 8, 2021
  • Skip unnecessary numbered_bases_to_rates, lettered_bases_to_rates.
  • Don't use pyplot, but pass axes to make_arrow_plot.
  • letter colors only depend on the start point, not the end point.
  • Don't hard-code coordinates of arrows, but compute them from the
    letter coordinates.
  • Only include one dataset.
  • Directly show all three possibilities (length, width, alpha).
  • head_starts_at_zero=True resulted in incorrect alignment e.g. when
    calling arrow_demo.py realistic length previously; one should use
    the default value of head_starts_at_zero=False.

Before, when called with arguments realistic length (note the completely incorrect alignment):
old
After:
new

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@QuLogic
Copy link
Member
QuLogic commented Jun 8, 2021

It seems like the text size has some meaning, but it also appears that only $C_3$ is any different. I wonder if there's any point to sizing those?

@@ -49,20 +38,14 @@ def make_arrow_plot(data, size=4, display='length', shape='right',
alpha : float
Maximum opacity of arrows.
**kwargs
Can be anything allowed by a Arrow object, e.g. *linewidth* or
Can be anything allowed by an Arrow object, e.g. *linewidth* or
Copy link
Member

Choose a reason for hiding this comment

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

Is there anyway to cross link to the ArrowStyle object here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linked

size = 4
plt.figure(figsize=(size, size))
fig = plt.figure(figsize=(3 * size, size), constrained_layout=True)
axs = fig.subplot_mosaic([["length", "width", "alpha"]])
Copy link
Member
@story645 story645 Jun 8, 2021

Choose a reason for hiding this comment

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

I find it really hard to learn these concepts from the demo (and I feel like @jklymak may also have opinions here).

I really like that you broke this out into three subplots but is there a way to make it clearer, especially given how much code is in this demo? Either by making the different length, width, alpha values very dramatic, and/or adding the values to the axes label?

Copy link
Member

Choose a reason for hiding this comment

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

I think its fine to have this stuff in here. The methods are clickable, and I don't think this is too complicated. OTOH I'd probably just do figsize=(12, 4); we can all see that it'll be 3x wider than tall ;-)

Copy link
Contributor 8000 Author

Choose a reason for hiding this comment

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

That size is also used later to scale the text size, so I left it in as a variable.

@anntzer
Copy link
Contributor Author
anntzer commented Jun 9, 2021

Re: text size: actually they're all differently sized, but the difference is a bit subtle. I increased the size of G to improve contrast (actually the sizes should probably be given by an eigenvector of the transition matrix, but let's not get there).

@anntzer anntzer force-pushed the arrow_demo branch 2 times, most recently from b488fce to 593ff94 Compare June 9, 2021 08:44
@timhoffm
Copy link
Member
timhoffm commented Jun 9, 2021

Directly show all three possibilities (length, width, alpha).

This is rather confusing without further context. I'm not convinced that this option is useful at all. It's definition and impelmentation are quite far apart. It complicates the code. And Having three different axes without any explanation is rather confusing.

I did not dig into the implementation, but I have the impression the author got carried away with making a fancy visualization, and the extra code obscures the actual demonstration of arrow usage. Possibly this should have been something much simpler in the first place.

I don't require to rewrite this whole thing, but as an incremental change

  • either ditch the display variants alltogether
  • or if you want to show all, write a sentence in the description that the different axes show certain variations in parameters and add titles "length modified", "width modified", "alpha modified" or similar.

@anntzer
Copy link
Contributor Author
anntzer commented Jun 9, 2021

I extended the description at the axes corner to "flux encoded as arrow {length,width,alpha}". I guess this may be a bit domain-specific but I thought that the example use case was actually quite clear.

@timhoffm
Copy link
Member
timhoffm commented Jun 9, 2021

may be a bit domain-specific

Is a bit of an understatement. I did not have any clue that the arrows were fluxes. And I still don't have any idea what this diagram shows - which in itself is not a problem because all I want to learn is how to draw arrows. But the description doesn't really help much.

I have the impression that this example is more of the "show fancy plot" category similar to
https://matplotlib.org/stable/gallery/lines_bars_and_markers/horizontal_barchart_distribution.html or https://matplotlib.org/stable/gallery/showcase/bachelors_degrees_by_gender.html
and not primarily an "Arrow Demo".

If this is some kind of standard domain specific plot, I suggest to rewrite the description to

[domain specific plot name]
===========================

This example illustrates how one can encode flux densities in [domian specific plot name]
using different `~.Axes.arrow` properties like, length, width or transparency."

Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Aside from the comments about the front matter this is an improvement on the current situation in my opinion. I agree that it should not get such a comprehensive name because it is really over the top for an arrow demo, and someone should devise a proper arrow demo, but until that time I personally think it should go in as more modern and more informative.

Comment on lines 6 to 7
Arrow drawing example for the new fancy_arrow facilities.

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
Arrow drawing example for the new fancy_arrow facilities.
Three ways of drawing arrows to encode strength of the arrow using length or width of the arrow,
or alpha of its color.

Original author: Rob Knight <rob@spot.colorado.edu>
"""
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
8000
Original author: Rob Knight <rob@spot.colorado.edu>
"""
"""

We don't cite people anymore..

- Skip unnecessary numbered_bases_to_rates, lettered_bases_to_rates.
- Don't use pyplot, but pass axes to make_arrow_plot.
- letter colors only depend on the start point, not the end point.
- Don't hard-code coordinates of arrows, but compute them from the
  letter coordinates.
- Only include one dataset.
- Directly show all three possibilities (length, width, alpha).
- head_starts_at_zero=True resulted in incorrect alignment e.g. when
  calling `arrow_demo.py realistic length` previously; one should use
  the default value of head_starts_at_zero=False.
@anntzer
Copy link
Contributor Author
anntzer commented Jun 10, 2021

I went mostly for @jklymak's suggestion, also mentioning that this can be used e.g. for Markov models (which is what I think this likely represented). You can find Markov model graphs on google, although most of them just write the numbers next to the arrows without varying arrow width/height/alpha (but that's likely just for simpler drawing...).

@jklymak
Copy link
Member
jklymak commented Jun 10, 2021

Anyone can merge after CI passes... I'll open #20409 an issue tracking the concerns with this being ov 8000 erly specialized...

@QuLogic QuLogic merged commit 84e0fab into matplotlib:master Jun 11, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone Jun 11, 2021
@anntzer anntzer deleted the arrow_demo branch June 11, 2021 05:48
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.

5 participants
0