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

Bug fix for parentheses getting dropped from cell definitions. #491

Merged
merged 29 commits into from
Aug 22, 2024

Conversation

MicahGale
Copy link
Collaborator

@MicahGale MicahGale commented Aug 16, 2024

Description

This fixes a bug where parentheses disappear from the geometry definition of a cell. This was because surfaces.HalfSpace essentially had no way to store that information, and when it was time to export the geometry would override the old parentheses.

This was solved in a few ways.

  1. Introduce Operation.GROUP to replace Operation._SHIFT in this context. This was because this will be more visible to users and GROUP is clearer. Also _SHIFT is used for certain leaf nodes in complex geometry, and lead to confusion.
  2. Adding an unbalanced tree level to HalfSpace to represent GROUP
  3. Updating HalfSapce to ensure that GROUP is properly handled on export.
  4. Made a method to detect when a group needs to be added for scratch geometries. This is done whenever a union node is below an intersection node. This is the proper order of operations (see MontePy deletes parentheses from cell regions #490).

To sanity check this logic:

    *
   / \
  +   3
 /\
1  2

Should be (1 : 2) 3 not 1 : 2 3

Fixes #490

Checklist

  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • Implement unit testing.
  • Clear out all TODO
  • Look into disappearing comments with @tjlaboss

@MicahGale
Copy link
Collaborator Author

Should we test writing all files out to file at least once and see what changes?

tests/test_edge_cases.py Outdated Show resolved Hide resolved
tests/test_edge_cases.py Outdated Show resolved Hide resolved
@tjlaboss
Copy link
Collaborator

Should we test writing all files out to file at least once and see what changes?

Eventually, we should test writing out each type of input at least once. This can be done piecemeal, with file read=... coming last.

@MicahGale
Copy link
Collaborator Author

Should we test writing all files out to file at least once and see what changes?

Eventually, we should test writing out each type of input at least once. This can be done piecemeal, with file read=... coming last.

I think this should wait until IO.StringIO is supported.

We already bulk read all valid test inputs through the checker. There's already good machinery for finding all valid input files. I think doing this enmass would be a good idea. We just run the risk of discovering a lot of bugs all at once that might is out of scope for this work I think.

@MicahGale MicahGale added bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". critical An issue that seriously limits user adoption or hampers current use. labels Aug 17, 2024
This was referenced Aug 17, 2024
@MicahGale MicahGale self-assigned this Aug 21, 2024
@MicahGale MicahGale marked this pull request as ready for review August 22, 2024 18:48
Copy link
Collaborator

@tjlaboss tjlaboss left a comment

Choose a reason for hiding this comment

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

NICE

@MicahGale MicahGale merged commit 980a825 into develop Aug 22, 2024
13 checks passed
@MicahGale MicahGale deleted the case_of_the_missing_parens branch August 22, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". critical An issue that seriously limits user adoption or hampers current use.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MontePy deletes parentheses from cell regions
2 participants