Expose streamlines #480

PProfizi · 2023-09-07T10:29:50Z

Create the ansys.dpf.post.tools package, with the ansys.dpf.post.tools.streamlines module.
Add the plot_streamlines helper to provide a first way of plotting streamlines based on a DataFrame.
Add a first test in test_streamlines.py.

Initial mock-up:

codecov · 2023-09-07T10:35:11Z

Codecov Report

Merging #480 (d67a405) into master (a713af2) will increase coverage by 0.27%.
Report is 1 commits behind head on master.
The diff coverage is 69.76%.

@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
+ Coverage   83.49%   83.77%   +0.27%     
==========================================
  Files          45       47       +2     
  Lines        5041     5072      +31     
==========================================
+ Hits         4209     4249      +40     
+ Misses        832      823       -9

rafacanton

@PProfizi Just made a couple of suggestions to avoid referring to velocity. This is way more general and applies to all vector fields. For example, when we support EMag data, the same method could be used to plot the magnetic field lines

src/ansys/dpf/post/tools/streamlines.py

Co-authored-by: Rafael Canton <[email protected]>

cbellot000 · 2023-09-08T06:07:25Z

src/ansys/dpf/post/tools/streamlines.py

+    if mesh:
+        meshed_region = mesh._meshed_region
+    else:
+        meshed_region = dataframe._fc[0].meshed_region


we will eventually need a public method in the dataframe to get the mesh, no?

@cbellot000 sure, but again it will need a label logic to select which field's meshed_region is of interest, or else I guess return a MeshesContainer as a Meshes

cbellot000 · 2023-09-08T06:08:26Z

src/ansys/dpf/post/tools/streamlines.py

+        meshed_region = mesh._meshed_region
+    else:
+        meshed_region = dataframe._fc[0].meshed_region
+    field = dataframe._fc[0]


why the first field? what if you have a single time, but different label (like "elshape") in your data frame?

@cbellot000 talked it out and it seems the only use-case is merging the fields labeled by zone, as well as selecting a set.
I updated the PR accordingly.

This reverts commit 6b8cdab.

…pf/post/

PProfizi · 2023-09-14T14:35:28Z

@rafacanton @cbellot000 @anslpa

Just realized we cannot actually create a ansys.dpf.post.tools sub-package with ansys.dpf.post.tools.streamlines module (ansys/dpf/post/tools/streamlines.py) as we already have a ansys.dpf.post.tools module (ansys/dpf/post/tools.py).
Four solutions:

rename the new sub-package from tools to helpers. This is the current solution implemented, so now the new method is in ansys.dpf.post.helpers.streamlines. This is also what was done in ansys-dpf-core which makes both libraries coherent.
Move the streamlines.py module directly in ansys/dpf/post
Move the streamlines helpers in the tools.py module, which would mean no streamlines-specific module and a direct call to from ansys.dpf.post.tools import plot_streamlines.
Remove the tools.py module and move its methods in a new module in the tools sub-package. This would mean makign sure that the old imports would still work. Not sure we can cover all edge-cases.

I think the first solution is the best.

What do you think?

rafacanton · 2023-09-15T08:16:14Z

@rafacanton @cbellot000 @anslpa

Just realized we cannot actually create a ansys.dpf.post.tools sub-package with ansys.dpf.post.tools.streamlines module (ansys/dpf/post/tools/streamlines.py) as we already have a ansys.dpf.post.tools module (ansys/dpf/post/tools.py). Four solutions:

rename the new sub-package from tools to helpers. This is the current solution implemented, so now the new method is in ansys.dpf.post.helpers.streamlines. This is also what was done in ansys-dpf-core which makes both libraries coherent.

Move the streamlines.py module directly in ansys/dpf/post

Move the streamlines helpers in the tools.py module, which would mean no streamlines-specific module and a direct call to from ansys.dpf.post.tools import plot_streamlines.

Remove the tools.py module and move its methods in a new module in the tools sub-package. This would mean makign sure that the old imports would still work. Not sure we can cover all edge-cases.

I think the first solution is the best.

What do you think?

@PProfizi I agree, I think it is right to have both libraries with a similar structure. Thanks!

…etro-compatibility while preparing deprecation.

…ect set in transient dataframe.

PProfizi · 2023-09-18T09:45:37Z

@rafacanton @cbellot000 @anslpa
Just realized we cannot actually create a ansys.dpf.post.tools sub-package with ansys.dpf.post.tools.streamlines module (ansys/dpf/post/tools/streamlines.py) as we already have a ansys.dpf.post.tools module (ansys/dpf/post/tools.py). Four solutions:

rename the new sub-package from tools to helpers. This is the current solution implemented, so now the new method is in ansys.dpf.post.helpers.streamlines. This is also what was done in ansys-dpf-core which makes both libraries coherent.

Move the streamlines.py module directly in ansys/dpf/post

Move the streamlines helpers in the tools.py module, which would mean no streamlines-specific module and a direct call to from ansys.dpf.post.tools import plot_streamlines.

Remove the tools.py module and move its methods in a new module in the tools sub-package. This would mean makign sure that the old imports would still work. Not sure we can cover all edge-cases.

I think the first solution is the best.
What do you think?

@PProfizi I agree, I think it is right to have both libraries with a similar structure. Thanks!

What I ended-up doing:

add a new ansys.dpf.post.helpers sub-package
add a new ansys.dpf.post.helpers.streamlines module
add a new ansys.dpf.post.helpers.selections module
moved the ansys.dpf.post.tools.select method to the ansys.dpf.post.helpers.selections module
made an import to it in ansys.dpf.post.tools to ensure retro-compatibility
updated the dosctring in ansys.dpf.post.tools to mention the move to ansys.dpf.post.helpers

cbellot000 · 2023-09-19T12:22:34Z

src/ansys/dpf/post/helpers/__init__.py

@@ -0,0 +1,6 @@
+"""Tools package.
+
+Tools


The only problem I see is naming. For me "helpers" means wrapping already existing capabilities in another way to make them easier to be used

Is there a plan for follow up PR adding an example for streamlines?

The only problem I see is naming. For me "helpers" means wrapping already existing capabilities in another way to make them easier to be used

IMO this is what this method is? The end-goal is to have a much more thorough streamlines manipulation capability, this method being a quick way to plot the streamlines given a Dataframe.

Is there a plan for follow up PR adding an example for streamlines?

Yes of course, I initially wanted this merged before last Friday's deadline, but if the deadline is not an issue anymore, I can add an example within this PR.

The only problem I see is naming. For me "helpers" means wrapping already existing capabilities in another way to make them easier to be used

@cbellot000 What I can do though is rename helpers as tools, but that will require deleting the tools.py module, which I think can be done without retro-compatibility issues if I import the select method from within the __init__.py file of the new tools sub-package, yet I may have missed a case where it breaks.

@PProfizi no, it's okay, your justification makes sense

cbellot000 · 2023-09-19T13:21:26Z

src/ansys/dpf/post/helpers/__init__.py

@@ -0,0 +1,6 @@
+"""Tools package.
+
+Tools


@PProfizi no, it's okay, your justification makes sense

PProfizi added 2 commits September 6, 2023 10:50

Small typing fix

7cf822b

Draft of ansys.dpf.post.tools.streamlines.plot_streamlines()

b1ab8b1

PProfizi added the enhancement New feature or request label Sep 7, 2023

PProfizi added this to the v0.5.1 milestone Sep 7, 2023

PProfizi requested a review from cbellot000 September 7, 2023 10:29

PProfizi self-assigned this Sep 7, 2023

PProfizi requested review from rafacanton and anslpa September 7, 2023 10:31

rafacanton reviewed Sep 7, 2023

View reviewed changes

PProfizi and others added 2 commits September 7, 2023 13:19

Apply suggestions from code review

ba53655

Co-authored-by: Rafael Canton <[email protected]>

Merge branch 'master' into feat/expose_streamlines

9557ca3

cbellot000 reviewed Sep 8, 2023

View reviewed changes

Implement remarks on sources definition

288fefd

rafacanton approved these changes Sep 11, 2023

View reviewed changes

PProfizi added 4 commits September 13, 2023 15:19

Remove __init__.py from ansys/dpf/post/tools

6b8cdab

Revert "Remove __init__.py from ansys/dpf/post/tools"

bae706b

This reverts commit 6b8cdab.

Add a conftest.py for Doctest

522aca1

Rename sub-package to "helpers" as tools.py already exists in ansys/d…

f5cc283

…pf/post/

PProfizi and others added 2 commits September 14, 2023 16:46

Merge branch 'master' into feat/expose_streamlines

2553d5d

Remove debug

11c6764

PProfizi added 3 commits September 15, 2023 14:07

Update pre-commit

d5577b1

Move the select method to a new helpers/selections module, ensuring r…

f317054

…etro-compatibility while preparing deprecation.

Handle multi-zone with a merge step, and add a set_id argument to sel…

d67a405

…ect set in transient dataframe.

PProfizi requested a review from cbellot000 September 18, 2023 13:53

cbellot000 reviewed Sep 19, 2023

View reviewed changes

cbellot000 approved these changes Sep 19, 2023

View reviewed changes

PProfizi merged commit f7dc485 into master Sep 20, 2023
21 of 23 checks passed

PProfizi deleted the feat/expose_streamlines branch September 20, 2023 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose streamlines #480

Expose streamlines #480

PProfizi commented Sep 7, 2023 •

edited

Loading

codecov bot commented Sep 7, 2023 •

edited

Loading

rafacanton left a comment

cbellot000 Sep 8, 2023

PProfizi Sep 8, 2023

cbellot000 Sep 8, 2023

PProfizi Sep 18, 2023

PProfizi commented Sep 14, 2023 •

edited

Loading

rafacanton commented Sep 15, 2023

PProfizi commented Sep 18, 2023

cbellot000 Sep 19, 2023

cbellot000 Sep 19, 2023

cbellot000 Sep 19, 2023

PProfizi Sep 19, 2023 •

edited

Loading

PProfizi Sep 19, 2023

cbellot000 Sep 19, 2023

cbellot000 Sep 19, 2023

Expose streamlines #480

Expose streamlines #480

Conversation

PProfizi commented Sep 7, 2023 • edited Loading

codecov bot commented Sep 7, 2023 • edited Loading

Codecov Report

rafacanton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PProfizi commented Sep 14, 2023 • edited Loading

rafacanton commented Sep 15, 2023

PProfizi commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PProfizi Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PProfizi commented Sep 7, 2023 •

edited

Loading

codecov bot commented Sep 7, 2023 •

edited

Loading

PProfizi commented Sep 14, 2023 •

edited

Loading

PProfizi Sep 19, 2023 •

edited

Loading