-
Notifications
You must be signed in to change notification settings - Fork 42
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
Post processing with pyVista #21
Conversation
ansys/fluent/session.py
Outdated
@@ -2,6 +2,8 @@ | |||
import threading | |||
|
|||
from ansys.api.fluent.v0 import datamodel_pb2_grpc as DataModelGrpcModule | |||
from ansys.api.fluent.v0 import fielddata_pb2_grpc as FieldGrpcModule | |||
from ansys.fluent.core.core import FieldDataService |
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.
These imports can be sorted. There is a tool isort to do that automatically.
This comment has been minimized.
This comment has been minimized.
ansys/fluent/core/core.py
Outdated
@@ -2,6 +2,7 @@ | |||
import os | |||
|
|||
from ansys.api.fluent.v0 import datamodel_pb2 as DataModelProtoModule | |||
from ansys.api.fluent.v0 import fielddata_pb2 as FieldDataProtoModule |
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 suggest we have a module per service which wraps that service's grpc calls, rather than importing the _pb2 files in many modules.
@@ -0,0 +1,2 @@ | |||
|
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.
Should we keep this file?
README.rst
Outdated
----------------- | ||
#import module |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
""" | ||
Instantiate the graphics objects. | ||
""" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
""" | ||
Displays mesh graphics. | ||
""" |
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.
Fix docstring. Public methods should include an example.
surface_ids = [ | ||
id | ||
for surf in obj.surfaces_list() | ||
for id in surfaces_info.get(surf, {}).get("surface_id", []) | ||
] |
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.
This seems inefficient. Is there a way this can be done through array access?
except Exception as e: | ||
print(e) | ||
pass |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
.keys() | ||
) | ||
if not "dummy" in surfaces_list: | ||
raise RuntimeError(f"Iso surface creation failed.") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This is a great start, and I'm impressed how far you've gotten with pyvista!
This PR needs unit testing and documentation, which I know is hard without a fluent docker image, but we're going to have to add it in. There's really no way to do test driven development without CI/CD performing PR validation; otherwise this isn't going to scale when we start adding significant features.
There's no better time to add in docs and unit testing than when implementing features.
…sys/pyfluent into feat/pyvista_postprocessing
cls, name, bases, attrs | ||
) | ||
|
||
|
||
class PyNamedObjectMeta(type): |
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 think we should introduce a container object.
Added containers to store collection.
Makefile
Outdated
install-pyvista-for-python3.10-linux: | ||
@pip install https://github.com/pyvista/pyvista-wheels/raw/main/vtk-9.1.0.dev0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl | ||
|
||
install-pyvista-for-python3.10-windows: | ||
@pip install https://github.com/pyvista/pyvista-wheels/raw/main/vtk-9.1.0.dev0-cp310-cp310-win_amd64.whl | ||
|
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.
Ha! You found the wheels.
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.
Docs are better, but we're still missing unit tests for this.
Even if we can't get unit tests for our CI/CD yet, we should still have unit tests that can be run locally.
@@ -1,5 +1,7 @@ | |||
[flake8] | |||
exclude = venv, tui.py, ansys/api/fluent | |||
per-file-ignores = |
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.
Interesting, what was causing the error. The naming looks ok. @ajain-ansys
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.
class names should use CapWords convention
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.
Yes and I'm not seeing why you need to ignore that error? It looks like you are using CapWords already. @ajain-ansys
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.
OK, I did not stare at it hard enough. It's because the member classes not following that naming convention.
Is there a reason the members are not following 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.
Member classes represent, object properties. We are not following CapWords convention for properties, because that is how we are accessing them. For example field for a contour object is accessed as: contour1.field.
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.
Right, ok, but I still don't understand. Is there any reason it can't be contour1.Field
? For instance, is there is an external requirement driving this design choice? @ajain-ansys
OK, sorry if I'm being dense, but Is it just that the other commands are all using lower case with underscores?
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.
Yes, that is correct. Other commands are all using lower case. For example, all commands in auto generated file tui.py are lower case.
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.
Yeah, very sorry, now I got it.
Multiple plotters support
Added a new module postprcessing/pyvista for pyFluent postprocessing. Currently mesh, contour and iso surfaces are supported.
After initialize and solve follow the steps: