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

Lucas/geometry refactor #1064

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Lucas/geometry refactor #1064

merged 1 commit into from
Aug 16, 2023

Conversation

lucas-flexcompute
Copy link
Collaborator

I tried to split our geometry primitives into separate files but there are a few interdependencies that seem impossible to avoid, so I ended up leaving Box, GeometryGroup and ClipOperation inside base.py.

  • Box is required as a return type for bounding_box, which is only defined for the base Geometry.
  • Geometry operations return GeometryGroup or ClipOperation, so those also have to be available for Geometry.

Furthermore, GeometryType and from_shapely are also problematic and were left on a separate file: utils.py. This file requires all geometry classes to define GeometryType.

  • GeometryType can only be defined after all classes, but it it used by GeometryGroup and ClipOperation, so a cycle is inevitable here if we want any file separation.
  • from_shapely should, ideally, be a method of Geometry because it can return different classes based on the shapely geometry, but it requires the construction of PolySlab, so it cannot depend only on base.py. Because we inevitably have a cyclic import due to GeometryType, I used the same cycle to define from_shapely outside base.py but wrap it inside Geometry.

The final organization is better than having a single file, but not as good as in the initial split (f08d821), which contained a lot more import cycles.

Only `GeometryType` and `from_shapely` remain as problematic objects
beacuse they require cyclic imports.

Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
@lucas-flexcompute lucas-flexcompute changed the base branch from lucas/set_operations to pre/2.4 August 16, 2023 12:58
@lucas-flexcompute lucas-flexcompute marked this pull request as ready for review August 16, 2023 12:58
@lucas-flexcompute lucas-flexcompute merged commit 9b64979 into pre/2.4 Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants