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

FEAT: implement ellipse and circle geometries #19

Merged
merged 4 commits into from
Sep 1, 2022
Merged

Conversation

jorgepiloto
Copy link
Member

@jorgepiloto jorgepiloto commented Aug 31, 2022

This branch can be merged once #16 is complete.

Example:

import matplotlib.pyplot as plt

from ansys.geometry.core.sketch.ellipse import EllipseSketch
from ansys.geometry.core.sketch.circle import CircleSketch


ellipse = EllipseSketch.from_axes(5, 2)
circle = CircleSketch.from_radius(2)

fig, ax = plt.subplots()
for sketch, color in zip([ellipse, circle], "rb"):
    ax.plot(sketch.x_coords, sketch.y_coords, "k")
    ax.plot(sketch.x_coords, sketch.y_coords, f"{color}o")
ax.set_aspect("equal")
plt.show()

Figure_2

@jorgepiloto
Copy link
Member Author

I will rebase as required for syncing this with its base branch.

@github-actions github-actions bot added maintenance Package and maintenance related enhancement New features or code improvements labels Aug 31, 2022
@jorgepiloto jorgepiloto force-pushed the feat/ellipse branch 2 times, most recently from 861ca0a to aa2e61b Compare August 31, 2022 14:36
@RobPasMue
Copy link
Member

You can update your branch @jorgepiloto =)

@RobPasMue
Copy link
Member

I will focus only on the sketch/* files since I see that there are a bunch of modifications on the primitives side.

@jorgepiloto
Copy link
Member Author

Let me improve the logic behind points generation for the circle and ellipse. I over complicated this too much.

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

I am missing some tests and improved docstrings for the classmethods but apart from that it is looking really good! =)

src/ansys/geometry/core/sketch/curve.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/sketch/curve.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/sketch/ellipse.py Outdated Show resolved Hide resolved
src/ansys/geometry/core/sketch/ellipse.py Outdated Show resolved Hide resolved
@jorgepiloto jorgepiloto force-pushed the feat/ellipse branch 4 times, most recently from b930f75 to eaefd73 Compare August 31, 2022 16:26
@RobPasMue RobPasMue mentioned this pull request Aug 31, 2022
27 tasks
@RobPasMue
Copy link
Member

This just looks great @jorgepiloto =)

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

LGTM @jorgepiloto

I'll leave it pre-approved so that it can be merged whenever you decide. But it is looking great.

Copy link
Contributor

@MaxJPRey MaxJPRey left a comment

Choose a reason for hiding this comment

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

It is great.
Thanks for offering us some math @jorgepiloto . 😃

@RobPasMue
Copy link
Member

I'll connect in an hour or so @jorgepiloto if you want some help with something. Otherwise feel free to merge whenever it is ready! =)

@jorgepiloto jorgepiloto force-pushed the feat/ellipse branch 3 times, most recently from 2de550f to 17156b5 Compare September 1, 2022 08:42
@jorgepiloto jorgepiloto marked this pull request as ready for review September 1, 2022 08:42
@jorgepiloto jorgepiloto changed the title FEAT: implement ellipse geometry FEAT: implement ellipse and circle geometries Sep 1, 2022
@jorgepiloto jorgepiloto merged commit 73ebb65 into main Sep 1, 2022
@jorgepiloto jorgepiloto deleted the feat/ellipse branch September 1, 2022 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or code improvements maintenance Package and maintenance related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants