-
Notifications
You must be signed in to change notification settings - Fork 25
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 geometry objects for plotting #691
Conversation
@PProfizi, @cbellot000, @germa89, @rlagha, feedback will be greatly appreciated! I still need to add some documentation and examples on the new classes, but I think this PR shouldn't include much more new stuff. |
Codecov Report
@@ Coverage Diff @@
## master #691 +/- ##
==========================================
+ Coverage 86.67% 87.17% +0.49%
==========================================
Files 69 71 +2
Lines 7760 8070 +310
==========================================
+ Hits 6726 7035 +309
- Misses 1034 1035 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! TBC!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
def plot(self, **kwargs): | ||
"""Visualize Points object.""" | ||
pl = DpfPlotter(**kwargs) | ||
pl.add_points(self._coordinates.data) | ||
pl.show_figure(show_axes=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this accept to input our own plotter?? Should we return the plotter somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the .plot()
method of each oject as a quick way for the user to check that this is indeed the object they intended to creats. What would you like to do when returning the plotter?
src/ansys/dpf/core/geometry.py
Outdated
class Line: | ||
""" | ||
Create Line object from two 3D points. | ||
|
||
Parameters | ||
---------- | ||
coordinates: array, list, Field, Points | ||
3D coordinates of the two points defining the line. | ||
num_points: int | ||
Number of points used to discretize the line. | ||
|
||
Examples | ||
-------- | ||
Create a line from two points, print object information and plot. | ||
|
||
>>> from ansys.dpf.core.geometry import Line | ||
>>> line = Line([[0, 0, 0], [1, 0, 0]]) | ||
>>> print(line) | ||
DPF Line object: | ||
Starting point: [0. 0. 0.] | ||
Ending point: [1. 0. 0.] | ||
Line discretized with 100 points | ||
>>> line.plot() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good docstrings!
src/ansys/dpf/core/geometry.py
Outdated
|
||
""" | ||
|
||
def __init__(self, coordinates, num_points=100, server=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont like to have num_points
hardcoded.... Also I dont see why we need to have the line discretized. I think it is for the intersection with other objects, but I would expect pyvista would take care of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to define the default value for discretizing the line. The main purpose now to discretize is not intersecting but to map field values and plot the field along this line. In fact, I think intersections should be done before, regardless of the discretizacion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with doing the intersection before.
If it is for mapping, I'm fine with that.
if np.isclose( | ||
np.abs(self.direction), normalize_vector(np.array([1, 1, 1])) | ||
).all(): | ||
camera_position = "xy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
pl = DpfPlotter(**kwargs) | ||
pl.add_line(self._coordinates.data) | ||
pl.show_figure(show_axes=True, cpos=camera_position) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concerns about points.plot
class Plane: | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice doc strings!
"""Create plane from three points.""" | ||
# Input check | ||
if isinstance(points, Points): | ||
if not len(points) == 3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me thing if we should have a method like point.is_in_plane(plane)
or better ... point in plane
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be useful, but I haven't seen where to use it yet.
src/ansys/dpf/core/plotter.py
Outdated
plane = pv.Plane( | ||
center=center, | ||
direction=direction, | ||
i_size=0.005, | ||
j_size=0.005, | ||
i_resolution=20, | ||
j_resolution=20, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use here the discretization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discretized objects are just to interpolate there later. Conceptually, I think a user should be able to see t'he plane even if It has nit been discretized.
Having said that, I was planning on changing this and discretize all objects right after discretizing. The reason being is that, for the planes, it makes a lot of sense asking to users to provide width and height of the plane when they try to create it using a center and a normal. However, if they define 3 points I think It would be more intuitive to consider these 3 points as the corners of the plane, thus, width and height being already defined.
What do you think?? Of course I will allow users to change the width and height of the plane anyway.
If I go with this approach, I Will be able to always plot the plane with its defined width and height and I will modify these lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discretize all objects right after discretizing
I'm not sure I follow.
asking to users to provide width and height of the plane when they try to create it using a center and a normal. However, if they define 3 points I think It would be more intuitive to consider these 3 points as the corners of the plane, thus, width and height being already defined.
This is not very clear to me. What do you mean with if they define 3 points I think It would be more intuitive to consider these 3 points as the corners of the plane, thus, width and height being already defined.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I have implemented now is that the objects are discretized right after initialization, so the user will not need to first create a plane and then discretize it. It will be done all in one line.
Regarding the plane. When creating a plane with its center and normal, it makes sense to ask the user to provide width and height of the line as:
plane = create_plane_from_center_and_normal(center=[0,0,0], normal=[0,1,0], width=1, height=1)
However, I think that when defining a plane with three points:
plane = create_plane_from_points([[0,0,0], [0,2,0], [0, 0, 4]])
the user may expect these three points to be the corner points of the plane. Thus, the width and height will be computed accordingly. For instance, the previous case will return a plane that has width = 2 and height = 4.
Co-authored-by: German <28149841+germa89@users.noreply.github.com>
…ydpf-core into feat/geometry-elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very useful new features Guillem, thank you!
5b63969
to
dd115d5
Compare
Fix #690.
This PR proposes a few basic geometric objects (points, lines and planes) to ease the user interaction when plotting sections, line plots, etc.
A test suite for the new implementation is provided.
Also, a plotting example has been added to demonstrate how to use these geometrical entities when plotting.