-
Notifications
You must be signed in to change notification settings - Fork 9
Two level structure for geometry referencing snappy #1465
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
base: snappy-main
Are you sure you want to change the base?
Conversation
name_components = patch.name.split("::") | ||
body_name = name_components[0] | ||
if body_name not in snappy_body_mapping: | ||
snappy_body_mapping[body_name] = [] |
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.
you can use default dict for initialization.
@property | ||
def snappy_bodies(self): | ||
"""Getter for the snappy registry.""" | ||
if hasattr(self, "snappy_body_registry") is False or self.snappy_body_registry is 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.
Since you changed the constructor to always populate self.snappy_body_registry as None, the first statement will overshadowed?
"""Reset the face grouping""" | ||
# pylint: disable=protected-access,no-member | ||
self.internal_registry = self._entity_info._reset_grouping("face", self.internal_registry) | ||
if hasattr(self, "snappy_body_registry") is True and self.snappy_body_registry: |
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 here?
"SurfaceGroupedEntityType", frozen=True | ||
) | ||
private_attribute_entity_type_name: Literal["SnappyBody"] = pd.Field("SnappyBody", frozen=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.
Please populate private_attribute_id too here.
from flow360.exceptions import Flow360ValueError | ||
|
||
|
||
class DoubleIndexableList(list): |
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 addition of this DoubleIndexableList seems redundant? See my other comment.
Get the full list of boundaries. | ||
If attribute_name is supplied then ignore stored face_group_tag and use supplied one. | ||
""" | ||
if self.face_group_tag == GROUPED_SNAPPY and attribute_name is 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.
Why do we need to add this if the faces are already grouped with faceId? Wouldn't the previous method give correct list of boundaries?
|
||
surfaces: List[Surface] = pd.Field() | ||
|
||
def __getitem__(self, key: str): |
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.
Why do we still need to get Surface
entities from SnappyBody?
To select a surface with pattern patch*
under SnappyBodies with pattern tunne*
we can just do:
my_geo["tunne*::patch*"]
Why do user want to instead do:
my_geo.snappy_body["tunne*"]["patch*"]
which is longer?
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.
Had a discussion with @piotrkluba and this is just a syntax candy for snappy user.
General idea:
Faces in snappy have "two layers" of groupings - bodies that are the equivalent of a SearchableSurfaceWithGaps entity in OpenFoam and regions which are similar to normal patches. The goal of this changes is to allow to reference both those levels by simple indexing, including wildcards.