Cleanup arrow_demo. #20390
Cleanup arrow_demo. #20390
Conversation
|
It seems like the text size has some meaning, but it also appears that only |
| @@ -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 | |||
There was a problem hiding this comment.
Is there anyway to cross link to the ArrowStyle object here?
| size = 4 | ||
| plt.figure(figsize=(size, size)) | ||
| fig = plt.figure(figsize=(3 * size, size), constrained_layout=True) | ||
| axs = fig.subplot_mosaic([["length", "width", "alpha"]]) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ;-)
There was a problem hiding this comment.
That size is also used later to scale the text size, so I left it in as a variable.
|
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). |
b488fce to
593ff94
Compare
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
|
|
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. |
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 If this is some kind of standard domain specific plot, I suggest to rewrite the description to |
There was a problem hiding this comment.
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.
| Arrow drawing example for the new fancy_arrow facilities. | ||
|
|
There was a problem hiding this comment.
| 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> | ||
| """ |
There was a problem hiding this comment.
| 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.
|
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...). |
|
Anyone can merge after CI passes... I'll open #20409 an issue tracking the concerns with this being overly specialized... |
letter coordinates.
calling
arrow_demo.py realistic lengthpreviously; one should usethe default value of head_starts_at_zero=False.
Before, when called with arguments
realistic length(note the completely incorrect alignment):After:
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).