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

Add 'connected' argument to 'Plotter.add_lines' to create a series of connected lines #4214

Merged
merged 9 commits into from
Apr 3, 2023

Conversation

p-j-smith
Copy link
Contributor

Overview

Add support for using Plotter.add_lines to create a series of connected lines from an array of points, i.e. the behaviour of this function prior to #3687

resolves #4065

Details

  • add a connected argument to Plotter.add_lines. If connected=True, the points passed to Plotter.add_lines will be taken to form a series of connected lines. The default is False to maintain the current behaviour of the function

  • add a test to check the correct number of line segments is created when setting connected=True

@github-actions github-actions bot added the enhancement Changes that enhance the library label Apr 1, 2023
@codecov
Copy link

codecov bot commented Apr 1, 2023

Codecov Report

Merging #4214 (b9686f1) into main (f49378e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4214   +/-   ##
=======================================
  Coverage   95.77%   95.77%           
=======================================
  Files          97       97           
  Lines       20687    20687           
=======================================
  Hits        19813    19813           
  Misses        874      874           

pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
pyvista/plotting/plotting.py Outdated Show resolved Hide resolved
@adeak
Copy link
Member

adeak commented Apr 2, 2023

@p-j-smith although we don't have an official policy about this, I recommend merging instead of rebasing and force pushing on PRs unless you have a strong opinion of your own in this regard :) We use squash merge so the local history on PRs is not a concern, and rebasing runs the risk of losing commits pushed by others (not a concern in your case probably, because you know what you're doing) and not being able to use the "changes since you last viewed" button on github that helps reviewing.

Copy link
Member

@adeak adeak 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 the contribution @p-j-smith. One comment, works for me with or without that.

pyvista/plotting/plotting.py Show resolved Hide resolved
@p-j-smith
Copy link
Contributor Author

I recommend merging instead of rebasing and force pushing on PRs

sorry about that! I do usually rebase and force push but I'm happy to merge in future pyvista prs

Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

@p-j-smith LGTM. Thanks for this. It is perfect!

@p-j-smith
Copy link
Contributor Author

Thanks for the fast reviews @adeak and @tkoyama010 ! Would one of you mind merging as I'm not authorised? Thanks!

@tkoyama010
Copy link
Member

We usually check that there are no objections 24 hours after the initial approval is given. This is to give accounts in all time zones a fair opportunity to comment. Already, I am going to merge because it has been 24 hours since the first approval.

@tkoyama010 tkoyama010 merged commit 57c0109 into pyvista:main Apr 3, 2023
21 checks passed
@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Changes that enhance the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for previous behaviour of Plotter.add_lines
3 participants