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

Fix issue with shortcut recompression #520

Merged
merged 75 commits into from
Sep 11, 2024

Conversation

MicahGale
Copy link
Collaborator

@MicahGale MicahGale commented Aug 28, 2024

Description

Bug fix #499

This fixes the bug from #499. This bug came down to a bug when two repeat shortcuts are touching. The second repeat assumes that the repeat node to its left is the value this one is repeating. To solve this the logic is updated to see if these edge values match.

Test Improvement #497

In the process I implemented testing where all valid input files are readin, "written" out and then re-read in, and the original and new files are compared. This found various padding bugs.

Bug fix: #489

For the bug: #489, interpolations (and all shortcuts) in geometry definitions were not recompressed. This was due to the fact that the GeometryTree had no idea that a shortcut was involved. This will be fixed by:

  1. Making it possible to have an interpolation of integer values only.
  2. Labeling GeometryTree accordingly when it comes from a shortcut
  3. Creating a method to recompress a shortcut in a GeoemtryTree.

Bug Fix #526

This is actually #352 in disguise.

Bug fix #352

This was due to the trailing comments not being found properly due to a few reasons. In #352 the issue was due to the default trees for cell modifiers being added. This makes it look like there were no trailing comments.

This case was fixed by flagging default tree items in the parameters, and ignoring them for transferring comments.

The other case found was not being transferred from a tally comment. In this case the padding was attached to the classifier. The get_trailing_comment logic had to be updated to allow falling through to that very first node in the tree. This issue was in part causing #526 due to the new line additions due to the comment being in the wrong object.

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • Update unit testing to be more thurough
  • Cover logic case of Jump Repeat
  • Implement more unit tests for geometry shortcuts.

@MicahGale MicahGale linked an issue Aug 28, 2024 that may be closed by this pull request
@MicahGale MicahGale added bugs A deviation from expected behavior that does not reach the level of being reportable as an "Error". CI/CD code improvement A feature request that will improve the software and its maintainability, but be invisible to users. labels Aug 28, 2024
@MicahGale
Copy link
Collaborator Author

MicahGale commented Aug 28, 2024

Not actually replicating #499 yet.

@tjlaboss
Copy link
Collaborator

The first expansion is ok. It seems to occur on the $N^\text{th}$ expansion on a line where $N>1$.

@MicahGale
Copy link
Collaborator Author

Awww. I see. Thanks.

@MicahGale
Copy link
Collaborator Author

This looks more correctly failing:

>               assert new_line == gold_line.rstrip().expandtabs(8)
E               AssertionError: assert 'imp:e   0 2r 2r' == 'imp:e   0 2r 1 r'
E
E                 - imp:e   0 2r 1 r
E                 ?              ^^
E                 + imp:e   0 2r 2r
E                 ?              ^

@MicahGale MicahGale self-assigned this Aug 28, 2024
montepy/input_parser/syntax_node.py Outdated Show resolved Hide resolved
tests/inputs/test_importance.imcnp Show resolved Hide resolved
tests/test_integration.py Outdated Show resolved Hide resolved
@MicahGale
Copy link
Collaborator Author

I accidentally stumbled upon #489 in this testing:

E               AssertionError: assert '10214   0    (1  2.03.04)' == '10214   0    (1  2i  4)'
E
E                 - 10214   0    (1  2i  4)
E                 ?                   ^^^
E                 + 10214   0    (1  2.03.04)
E                 ?                   ^^^^^

(this is for tests/inputs/test_interp_edge.imcnp).

@MicahGale
Copy link
Collaborator Author

Ohh I see it now, I focused on the wrong problem there.

@MicahGale MicahGale marked this pull request as ready for review September 11, 2024 13:11
@MicahGale
Copy link
Collaborator Author

Ok your bug with regards to Importance seems to be solved.

This had to be manually addressed because importance is a special snow-flake and has its own system for handling syntax trees.

@MicahGale
Copy link
Collaborator Author

And... it broke again.

@MicahGale MicahGale merged commit 8329afe into develop Sep 11, 2024
15 checks passed
@MicahGale MicahGale deleted the 499-shortcuts-recompressed-incorrectly branch September 11, 2024 15:51
@MicahGale MicahGale mentioned this pull request Sep 23, 2024
3 tasks
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". CI/CD code improvement A feature request that will improve the software and its maintainability, but be invisible to users. critical An issue that seriously limits user adoption or hampers current use.
Projects
None yet
2 participants