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

Reducing pymapdl-reader dependency on the mesh module #1299

Merged
merged 40 commits into from
Aug 30, 2022
Merged

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Jul 27, 2022

This will remove the dependency of the mapdl.mesh module on PyMAPDL Reader (ansys-mapdl-reader).

The main issue here is parsing the RST file to a VTK file which can be then be used in Pyvista for plotting.

In theory, if we are not plotting, we don't need to parse. I shall make sure this is something extra (removing dependencies, lazy imports, etc).

@germa89 germa89 self-assigned this Jul 27, 2022
@germa89
Copy link
Collaborator Author

germa89 commented Jul 27, 2022

The last commit was by mistake.

@germa89
Copy link
Collaborator Author

germa89 commented Jul 27, 2022

By the way, I implemented the _parse_to_vtk function of reader to check everything else works. Still working on this...

@germa89 germa89 mentioned this pull request Jul 27, 2022
4 tasks
@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #1299 (14287b7) into main (f6c88a0) will increase coverage by 0.73%.
The diff coverage is 91.60%.

❗ Current head 14287b7 differs from pull request most recent head 983eca9. Consider uploading reports for the commit 983eca9 to get more accurate results

@@            Coverage Diff             @@
##             main    #1299      +/-   ##
==========================================
+ Coverage   76.76%   77.50%   +0.73%     
==========================================
  Files          43       44       +1     
  Lines        6878     7104     +226     
==========================================
+ Hits         5280     5506     +226     
  Misses       1598     1598              

@koubaa
Copy link
Contributor

koubaa commented Jul 28, 2022

@germa89 its not correct to use DPF to replace the reader in mesh_grpc.

Prior to this change, it is possible to use mesh_grpc to get the mesh without solving. When using dpf, there must be a result file in order for dpf to get a mesh. As it stands, I believe we are losing important functionality.

@koubaa koubaa marked this pull request as ready for review July 28, 2022 15:41
@akaszynski
Copy link
Collaborator

@germa89 its not correct to use DPF to replace the reader in mesh_grpc.

Prior to this change, it is possible to use mesh_grpc to get the mesh without solving. When using dpf, there must be a result file in order for dpf to get a mesh. As it stands, I believe we are losing important functionality.

I think we should separate out the mesh reading component from the result post-processing component from ansys-mapdl-reader. In fact, we still don't have an archive reader, so it's something critical that ansys-mapdl-reader will still provide even with DPF, until they add it as an operator.

@germa89
Copy link
Collaborator Author

germa89 commented Aug 22, 2022

I think what @koubaa is quite important. Hence we should not really relay on the DPF (or at least only).

After thinking about it, and examining DPF core VTK functions, I think the best approach is to externalize the whole thing (VTK export).

I have open a discussion in here: ansys/pydpf-core#421

If successful, we will use this new library to build the dynamic mesh (from the MAPDL instance), and also to build the mesh from the RST file.

src/ansys/mapdl/core/reader/mesh.py Outdated Show resolved Hide resolved
@germa89 germa89 marked this pull request as draft August 23, 2022 19:13
@germa89 germa89 mentioned this pull request Aug 24, 2022
2 tasks
@germa89 germa89 marked this pull request as ready for review August 29, 2022 14:41
@germa89 germa89 requested review from koubaa and akaszynski August 29, 2022 14:41
Copy link
Contributor

@koubaa koubaa left a comment

Choose a reason for hiding this comment

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

lgtm!

@akaszynski
Copy link
Collaborator

Only issue I see from this is the potential for duplication. If we plan to depreciate pymapdl-reader anyway, there won't be duplication and we can keep the mesh logic here in pymapdl. We'll still need some C code somewhere to allow rapid parsing, but that can be a ansys.mapdl.mesh library.

@germa89
Copy link
Collaborator Author

germa89 commented Aug 30, 2022

Only issue I see from this is the potential for duplication. If we plan to depreciate pymapdl-reader anyway, there won't be duplication and we can keep the mesh logic here in pymapdl. We'll still need some C code somewhere to allow rapid parsing, but that can be a ansys.mapdl.mesh library.

I see two options here:

  • Add another library (ansys.mapdl.mesh for example) which contain the whole Mesh object (including parsing).
  • Add another library (ansys.helpers/tools.vtk_export) which parse the MAPDL FEA model to a VTK object.

I have been advocating for the latest, because it can be reused in Prime and DPF.

References:

ansys/pydpf-core#421

@akaszynski
Copy link
Collaborator

I have been advocating for the latest, because it can be reused in Prime and DPF.

Agreed. Let's implement that as a standalone. I don't want this to be dependent on DPF; it just needs to be a simple C library with a python wrapper.

Copy link
Collaborator

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Approving. Test failure appears to be a service launch failure. Recommend fixing in a follow-up.

  Error response from daemon: Container 7c73f2d58b7da19bce19438a6601c20fdea363faee2f10da3f98f71952888fd5 is restarting, wait until the container is running

I have seen that message quite a lot lastly... but I have a plan.

@germa89 germa89 merged commit 443f68b into main Aug 30, 2022
@germa89 germa89 deleted the feat/replacing-mesh branch August 30, 2022 17:24
@koubaa
Copy link
Contributor

koubaa commented Aug 30, 2022

Only issue I see from this is the potential for duplication. If we plan to depreciate pymapdl-reader anyway, there won't be duplication and we can keep the mesh logic here in pymapdl. We'll still need some C code somewhere to allow rapid parsing, but that can be a ansys.mapdl.mesh library.

I see two options here:

  • Add another library (ansys.mapdl.mesh for example) which contain the whole Mesh object (including parsing).
  • Add another library (ansys.helpers/tools.vtk_export) which parse the MAPDL FEA model to a VTK object.

I have been advocating for the latest, because it can be reused in Prime and DPF.

References:

pyansys/pydpf-core#421

I'm a bit skeptical that a C library that takes as input an mapdl mesh as arrays can be reused by DPF or prime.

@germa89
Copy link
Collaborator Author

germa89 commented Aug 30, 2022

Only issue I see from this is the potential for duplication. If we plan to depreciate pymapdl-reader anyway, there won't be duplication and we can keep the mesh logic here in pymapdl. We'll still need some C code somewhere to allow rapid parsing, but that can be a ansys.mapdl.mesh library.

I see two options here:

  • Add another library (ansys.mapdl.mesh for example) which contain the whole Mesh object (including parsing).
  • Add another library (ansys.helpers/tools.vtk_export) which parse the MAPDL FEA model to a VTK object.

I have been advocating for the latest, because it can be reused in Prime and DPF.

References:

pyansys/pydpf-core#421

I'm a bit skeptical that a C library that takes as input an mapdl mesh as arrays can be reused by DPF or prime.

We should probably then discuss with them what are their needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Related with CICD, Github Actions, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants