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

Homogenizing geometry API #2125

Merged
merged 51 commits into from
Jul 25, 2023
Merged

Homogenizing geometry API #2125

merged 51 commits into from
Jul 25, 2023

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Jun 16, 2023

Attemp to homogenise the geometry API. At the moment, different methods keypoints, lines, areas, etc return different objects.

With this PR, all methods should return a pyvista.MultiBlock object which can be easily plotted, or even iterate to. This establishes a deeper connection with the pyvista package.

Discussion in here: #2125 (comment)

@github-actions github-actions bot added Enhancement Improve any current implemented feature New Feature Request or proposal for a new feature labels Jun 16, 2023
@germa89 germa89 requested a review from Gryfenfer97 June 16, 2023 11:26
@germa89
Copy link
Collaborator Author

germa89 commented Jun 16, 2023

@Gryfenfer97 can you have a look at the typing part? i am no expert so I would appreciate a lot your comments .

@germa89 germa89 self-assigned this Jun 16, 2023
@germa89 germa89 added the DO NOT MERGE Not ready to be merged yet label Jun 16, 2023
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
Co-authored-by: Jamil Hajjar <hajjarjamil4@gmail.com>
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
src/ansys/mapdl/core/mapdl_geometry.py Outdated Show resolved Hide resolved
@germa89 germa89 marked this pull request as draft July 3, 2023 10:41
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #2125 (32c5b00) into main (997e288) will increase coverage by 6.49%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##             main    #2125      +/-   ##
==========================================
+ Coverage   80.82%   87.32%   +6.49%     
==========================================
  Files          44       45       +1     
  Lines        7489     8203     +714     
==========================================
+ Hits         6053     7163    +1110     
+ Misses       1436     1040     -396     

@AlejandroFernandezLuces
Copy link
Contributor

Regarding PyVista, you can use PointSet for points, PolyData for lines and UnstructuredGrid for any other geometry. I see that you need to separate PolyData and UnstructuredGrid because of previous issues, so no discussion there.

My only commet is regarding the PointSet. If you look at the docs:

This class is useful for improving the performance of filters on point clouds, but not plotting.

If you don't really need to use if for that reason, you could also use PolyData for points, to increase compatibility among returned objects. This is not a big deal anyway, so up to you 🙂

Partial comments

Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
germa89 and others added 2 commits July 18, 2023 11:29
Second part.

Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@germa89
Copy link
Collaborator Author

germa89 commented Jul 18, 2023

Regarding PyVista, you can use PointSet for points, PolyData for lines and UnstructuredGrid for any other geometry. I see that you need to separate PolyData and UnstructuredGrid because of previous issues, so no discussion there.

My only commet is regarding the PointSet. If you look at the docs:

This class is useful for improving the performance of filters on point clouds, but not plotting.

If you don't really need to use if for that reason, you could also use PolyData for points, to increase compatibility among returned objects. This is not a big deal anyway, so up to you 🙂

I dont think we do much filtering with keypoints but we do a lot of plotting. So I guess choosing pv.PolyData is good and consistent with the homogenisation perspective.

Thank you for your feedback!

@germa89 germa89 mentioned this pull request Jul 20, 2023
4 tasks
germa89 and others added 2 commits July 21, 2023 12:29
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@germa89
Copy link
Collaborator Author

germa89 commented Jul 24, 2023

@clatapie

@germa89 germa89 merged commit c8f75b7 into main Jul 25, 2023
22 checks passed
@germa89 germa89 deleted the feat/homogenizing-geometry-API branch July 25, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve any current implemented feature New Feature Request or proposal for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants