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

Alpha argument added to BezierTriangle and CurvedPolygon plotting functions #296

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

BryceStansfield
Copy link
Contributor

No description provided.

@@ -62,14 +62,15 @@ def add_plot_boundary(ax, padding=0.125):
)


def add_patch(ax, color, pts_per_edge, *edges):
def add_patch(ax, color, pts_per_edge, *edges, alpha=None):
Copy link
Owner

Choose a reason for hiding this comment

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

This should just be

Suggested change
def add_patch(ax, color, pts_per_edge, *edges, alpha=None):
def add_patch(ax, color, pts_per_edge, *edges, alpha=0.625):

based on the usage below, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the rest of the changes, I think it's fine to default to alpha=0.625 everywhere.

Though I appreciate why you went with alpha=None as the API choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Np, I'll switch over to 0.625 then

"""Add a polygonal surface patch to a plot.

Args:
ax (matplotlib.artist.Artist): A matplotlib axis.
color (Tuple[float, float, float]): Color as RGB profile.
pts_per_edge (int): Number of points to use in polygonal
approximation of edge.
alpha (Optional[float]): Alpha value of patch centre, between 0 and 1 inclusive.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
alpha (Optional[float]): Alpha value of patch centre, between 0 and 1 inclusive.
alpha (Optional[float]): Alpha value of patch center, between 0 and 1 inclusive.

American spelling? For consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yep, thanks!

I'm an aussie, so that spelling was mostly muscle memory for me

Copy link
Owner

Choose a reason for hiding this comment

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

No worries!

Copy link
Owner

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

Thanks a ton for doing this!

"""Plot the current curved polygon.

Args:
pts_per_edge (int): Number of points to plot per curved edge.
color (Optional[Tuple[float, float, float]]): Color as RGB profile.
ax (Optional[matplotlib.artist.Artist]): matplotlib axis object
to add plot to.
alpha (Optional[float]): Alpha value of patch centre, between 0 and 1 inclusive.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
alpha (Optional[float]): Alpha value of patch centre, between 0 and 1 inclusive.
alpha (Optional[float]): Alpha value of patch center, between 0 and 1 inclusive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get this one too, thanks!

@@ -62,14 +62,15 @@ def add_plot_boundary(ax, padding=0.125):
)


def add_patch(ax, color, pts_per_edge, *edges):
def add_patch(ax, color, pts_per_edge, *edges, alpha=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Looking at the rest of the changes, I think it's fine to default to alpha=0.625 everywhere.

Though I appreciate why you went with alpha=None as the API choice.

@BryceStansfield
Copy link
Contributor Author

I think I've got all your comments, best of luck and thanks for the library, I've been finding it super helpful!

Copy link
Owner

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

LGTM

@dhermes
Copy link
Owner

dhermes commented Aug 10, 2022

The lint failures are "my fault" (just dependencies changing since the repo was last updated) and we can merge with them failing.

@dhermes dhermes merged commit a90a290 into dhermes:main Aug 10, 2022
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.

2 participants