8000 Connect stream lines if no varying width or color by gokberkgunes · Pull Request #25112 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Connect stream lines if no varying width or color #25112

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 4, 2023
Merged

Connect stream lines if no varying width or color #25112

merged 3 commits into from
Feb 4, 2023

Conversation

gokberkgunes
Copy link
Contributor
@gokberkgunes gokberkgunes commented Jan 31, 2023 •
edited
Loading

PR Summary

Streamplot segments stream lines into small pieces of lines in order to have different width and colors from each other. However, by doing the segmentation the connection of lines are getting lost. The loss of connection introduces visible gaps. This commit avoids unnecessary segmentation of a streamline into smaller lines if there is no need to do so. Fixing the problem only for non-varying width and color stream plots.

This commit is based on the comment of @QuLogic.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@gokberkgunes gokberkgunes changed the title Connect stream lines if no varying length or color Connect stream lines if no varying width or color Jan 31, 2023
@gokberkgunes
Connect stream lines if no varying width or color
9023040 
Streamplot segments stream lines into small pieces of lines in order to
have different width and colors from each other. However, by doing the
segmentation the connection of lines are getting lost. The loss of
connection introduces visible gaps. This commit avoids unnecessary
segmentation of a streamline into smaller lines if there is no need to
do so. Fixing the problem only for non-varying width and color stream plots.
@tacaswell tacaswell added this to the v3.8.0 milestone Jan 31, 2023
@tacaswell
Copy link
Member

Thank you for picking this up @gokberkgunes !

We are going to have to regenerate a bunch of test images (I do not think it is worth putting a back-compat flag in for this). It will also need a behavior change note

@ksunden
Copy link
Member
ksunden commented Jan 31, 2023

It's actually surprisingly few images that need to be remade, it seems (at least there are only 4 tests that fail), but still, we will need to regenerate those.

@gokberkgunes
Copy link
Contributor Author
gokberkgunes commented Feb 1, 2023 •
edited
Loading

I am not very experienced with tests. I have read the documentation and checked some other pull requests.

Before going further I wanted to ask you if what I am thinking is correct or not. So, should I directly regenerate and replace at least the failing streamplot test images which are in tests/baseline_images/test_streamplot? Additionally, should I do this replacement under this pull request?

@rcomer
Copy link
Member
rcomer commented Feb 2, 2023

Hi @gokberkgunes yes, please replace the images within this pull request. Since all the failures are in a single test module, I would run that one module with

pytest lib/matplotlib/tests/test_streamplot.py

the new versions of the images should then appear in result_images/test_streamplot/. For the ones that failed, you can then copy them over to the baseline images directory, as you say.

To check it worked, you can run the test module again and this time the tests should pass.

@gokberkgunes
Regenerate test images for no varying width/color
05983e1 
This pull requests regenerates the failing images for tests. The
regenerated images either have a non-varying widths or non-varying
colors.
@tacaswell
Copy link
Member

https://github.com/matplotlib/matplotlib/blob/main/tools/triage_tests.py is also very helpful as it will show you the images and has an "accept" button that does the copying for you.

tacaswell
tacaswell approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member
@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

These outputs are unambiguously better.

I am 50/50 on if this should get a behavior change note in case it catches out any downstream libraries that are using this method and will need to update their tests accordingly.

@gokberkgunes
Copy link
Contributor Author
gokberkgunes commented Feb 2, 2023 •
edited
Loading

@rcomer, thank you for the detailed directions.

@tacaswell, I would agree on adding a behavior change note. As you say, it would avoid headache to those who use tests.

Additionally, since we are dealing with #24946, I would like to ask that is it sensible to add an argument to streamplot that allows changing the cap style? Without enforcing a change on streamplot users unlike #25078; this would allow people to produce visually gapless plots if they wish to use round cap styles.

@tacaswell
Copy link
Member

I would like to ask that is it sensible to add an argument to streamplot that allows changing the cap style?

Giving the code a very quick skim, I think a better extension would be to make it possible to pass any line stlye keywords through (not just cap). We probably should follow the pattern of adding an option like ax.streamplot(..., linestyle_kw={'cap': round', ...}) (the other option is to collect any **kwargs and use those as the line kwargs, we have examples of both.....there might be a discussion to be had there) rather than playing whack-a-mole and adding them one at a time. There would have to be some de-confliction with the existing arguments as well (e.g. color and width).

https://github.com/matplotlib/matplotlib/tree/main/doc/api/next_api_changes/behavior <- the behavior change note should go there, please see the 00001-ABC.rst for a template.

@ksunden ksunden merged commit 1125888 into matplotlib:main Feb 4, 2023
@rcomer rcomer mentioned this pull request Feb 4, 2023
Open
@rcomer
Copy link
Member
rcomer commented Feb 4, 2023

Congratulations on your first PR merged into Matplotlib @gokberkgunes! We hope to hear from you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@gokberkgunes @tacaswell @ksunden @rcomer
0