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.

Sign up for GitHub

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

Fix for #203, errors in simplify full #206

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Fix for #203, errors in simplify full #206

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a122030
Merge pull request #175 from GEOUNED-org/dev
AlvaroCubi May 16, 2024
353f6e7
Patch for #203
Jun 3, 2024
291f33b
Merge branch 'dev' of github.com:KBGrammer/GEOUNED into dev
Jun 3, 2024
1dc65a4
Responding to comments on #206
Jun 3, 2024
c54a129
Update core.py
KBGrammer Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/geouned/GEOUNED/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,8 +681,8 @@ def start(self):
if c.Definition.level == 0 or c.IsEnclosure:
continue
logger.info(f"simplify cell {c.__id__}")
Box = UF.get_box(c)
CT = build_c_table_from_solids(Box, (c.Surfaces, Surfs), option="full")
Box = UF.get_box(c,self.options)
AlvaroCubi marked this conversation as resolved.
Show resolved Hide resolved
CT = build_c_table_from_solids(Box, (c.Surfaces, Surfs), option="full",options=self.options)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CT = build_c_table_from_solids(Box, (c.Surfaces, Surfs), option="full",options=self.options)
CT = build_c_table_from_solids(Box, (c.Surfaces, Surfs),options=self.options)

here I would prefer to pass only the self.options class as argument.

Also to avoid mistake the option attribute name of the self.options class should be changed from option to simplification_mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm missing something, it doesn't look like just self.options will work. I believe simplify is contained in self.settings.simplify.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right. The quick review made me make a mistake.

c.Definition.simplify(CT)
c.Definition.clean()
if type(c.Definition.elements) is bool:
Expand Down
4 changes: 2 additions & 2 deletions src/geouned/GEOUNED/utils/boolean_solids.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent, let the name of the variable in lines 442 and 481 be cut_box and rename the function in lines 463, 475 and 518 to divide_box

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.

I've made this change. I also noticed that the same issue with a variable sharing the function name cut_box appears in GEOReverse as well, so I've made the same changes there.

Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ def point_inside(solid):
return point

cut_line = 32
cut_box = 2
cutbox = 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cutbox = 2
cut_box = 2


v1 = solid.Vertexes[0].Point
for vi in range(len(solid.Vertexes) - 1, 0, -1):
Expand Down Expand Up @@ -478,7 +478,7 @@ def point_inside(solid):
boxes = subbox
n = n + 1

if n == cut_box:
if n == cutbox:
AlvaroCubi marked this conversation as resolved.
Show resolved Hide resolved
break

return point_from_surface(solid)
Expand Down