search

idaholab / MontePy

MontePy is the most user friendly Python library (API) to read, edit, and write MCNP input files.
https://www.montepy.org/
MIT License
30 stars 6 forks source link

Fix issue with shortcut recompression #520

Closed MicahGale closed 2 weeks ago

MicahGale commented 1 month ago

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

MicahGale commented 1 month ago

Not actually replicating #499 yet.

tjlaboss commented 1 month ago

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

MicahGale commented 1 month ago

Awww. I see. Thanks.

MicahGale commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

The ShortcutNode -> GeometryTree -> ShortcutNode transform is a bit lossy. The question is: is this acceptable for now?

Top/right side is the original input. 

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

Edit: now tracked in #527.

tjlaboss commented 1 month ago

Yes.

  1. Make it accurate
  2. Make it valid
  3. Make it pretty
MicahGale commented 1 month ago

Also found: #526, #524

tjlaboss commented 4 weeks ago

original

MCNP Test Model
C Cells
C Graveyard
99    0 +1010
C Seven spheres within a sphere
1 1 -10 -1001
2 1 -10 -1002
3 1 -10 -1003
4 1 -10 -1004
5 1 -10 -1005
6 1 -10 -1006
7 1 -10 -1007
8     0 -1010  1001  1002  1003  1004  1005  1006  1007

C surfaces
1001 SO      0.1
1002 SX -1.1 0.1
1003 SX +1.1 0.1
1004 SY -1.1 0.1
1005 SY +1.1 0.1
1006 SZ -1.1 0.1
1007 SZ +1.1 0.1
1010 SO 3

C data
C Let the importances be equal.
imp:n 0 1 2 3  10 12 14 16 1
C Second linear interpolation recompressed incorrectly
imp:p 0 2i 3   10 2i 16    1
C Repeat 
VOL   J 1 2R 2 10 2R 20    
C materials
C UO2 5 atpt enriched
m1        92235.80c           5
          92238.80c          95
C execution
ksrc 0 0 0
kcode 100000 1.000 50 1050
mode n p

result

MCNP Test Model
C Cells
C Graveyard
99    0 +1010
C Seven spheres within a sphere
1 1 -10 -1001
2 1 -10 -1002
3 1 -10 -1003
4 1 -10 -1004
5 1 -10 -1005
6 1 -10 -1006
7 1 -10 -1007
8     0 -1010  1001  1002  1003  1004  1005  1006  1007

C surfaces
1001 SO      0.1
1002 SX -1.1 0.1
1003 SX +1.1 0.1
1004 SY -1.1 0.1
1005 SY +1.1 0.1
1006 SZ -1.1 0.1
1007 SZ +1.1 0.1
1010 SO 3

C data
C Let the importances be equal.
imp:n 0 1 2 3  10 12 14 16 1
C Second linear interpolation recompressed incorrectly

imp:p 0 2i 3   3i 16    1
C Repeat

C Repeat
VOL   J 1 2R 2 10 2R 20
C materials
C UO2 5 atpt enriched
m1        92235.80c           5
          92238.80c          95
C execution
ksrc 0 0 0
kcode 100000 1.000 50 1050
mode n p
MicahGale commented 4 weeks ago

Can decide if coverage needs to be increased: https://coveralls.io/builds/69589075

141 of 144 new or added lines in 6 files covered. (97.92%)

1 existing line in 1 file now uncovered. 6146 of 6265 relevant lines covered (98.1%)

0.98 hits per line

MicahGale commented 3 weeks ago

526 is being tested for. Did you try it with something else?

MicahGale commented 3 weeks ago

Need to test multiple consecutive shortcuts of all types. For example, the following linear interpolation is expanded incorrectly.

< imp:p 0 2i 3   10 2i 16    1
---      
> imp:p 0 2i 3   3i 16    1

526 also persists. I'll post an example.

I think I covered this one right?

tjlaboss commented 3 weeks ago

Try reading and rewriting this input: https://github.com/idaholab/MontePy/pull/520#issuecomment-2325199121

I'm using commit 843db181452f29c36780ec4094273c1df1d0732d

MicahGale commented 3 weeks ago

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

MicahGale commented 2 weeks ago

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 commented 2 weeks ago

And... it broke again.