Fixed a bug in distribcell offsets logic

[GuySten]

Description

This pr fixes a bug in distribcell offset logic.

I have added checks that distribcell paths are unique for every distribcell instance.

I then changed the distribcell offset logic to pass all regression tests and the new added checks.

closes shaikinast/SPERT#8

Checklist

• [x] I have performed a self-review of my own code
• [x] I have run clang-format (version 15) on any C++ source files (if applicable)
• [x] I have followed the style guidelines for Python source files (if applicable)
• [x] I have made corresponding changes to the documentation (if applicable)
• [x] I have added tests that prove my fix is effective or that my feature works (if applicable)

[lewisgross1296]

What exactly was going wrong here / do you have a test or MWE that shows the change in behavior? Going from incorrect to correct behavior, for example.

I think I remember having issues with Distribcell Filters in the past and am curious if this is related. If there's an existing issue related to this PR, it would be good to have a Closes #ISSUE as well.

[GuySten]

I found the issue by debugging why a huge reactor model crashed when using distribmats. I traced the bug to the fact that in that model the function cell_instance_at_level from geometry.cpp returned a value that is bigger than the total number of instances (obviously wrong behaviour). I then discovered that the function distribcell_path crashes sometimes so I included a (failing) check that for every distribcell instance a unique distribcell_path can be computed. I then fixed the logic to make this check work and to pass all the rest of the regression tests.

The fact that the regression tests pass with my added check (that was initially failing) is the proof that the implementation is correct.

Except for the huge reactor model I don't know about an issue that this solves.

[lewisgross1296]

Neat, glad you were able to get past your issue and uncover something that was going wrong.

I asked about tests because I didn't see an added test in the Files changed tab and I'm not quite sure I see where the regression tests were failing on Github Actions (at least for this PR, attached a screenshot of the only two instances of this PR in the GHA tab)

Was it a local failure for the regression test that you were experiencing?

[GuySten]

I had local regression tests failing after adding the lines: https://github.com/openmc-dev/openmc/blob/6eab5dbdee234d104b974b7006d03cb39a4cc688/src/geometry_aux.cpp#L434C1-L455C2

Specifically I remember that the distribmat regression test was failing after adding the check above.

[GuySten]

@pshriwise, you have access to shaikinast/SPERT#8 so maybe you can review this pr?

[GuySten]

I thought about another simplification to the distribcell logic. We can make cell.n_instances a function that will call universe.n_instances and store universe instance count instead of cell instance count. This optimization will reduce storage of instance numbers when there are alot of cells.

@pshriwise, If you think it is worthwhile I can implement it in this pr.

[GuySten]

Based on the MWE you made, it could be reuse of the same universe in different lattices. Is that right?

I arrived at the MWE based on the assumption that it is true. So I suspect that it is right.

[pshriwise]

I thought about another simplification to the distribcell logic. We can make cell.n_instances a function that will call universe.n_instances and store universe instance count instead of cell instance count. This optimization will reduce storage of instance numbers when there are alot of cells.

@pshriwise, If you think it is worthwhile I can implement it in this pr.

I like it! I'll suggest we come to a resolution on some other things in this PR first, but I'd like to revisit for sure.

[GuySten]

I thought about another simplification to the distribcell logic. We can make cell.n_instances a function that will call universe.n_instances and store universe instance count instead of cell instance count. This optimization will reduce storage of instance numbers when there are alot of cells. @pshriwise, If you think it is worthwhile I can implement it in this pr.

I like it! I'll suggest we come to a resolution on some other things in this PR first, but I'd like to revisit for sure.

In my last commit I removed Cell.n_instances and now it resides in model::universe_counts.

[GuySten]

I think that this PR is ready or nearly ready to be merged. @pshriwise , do you agree?

[GuySten]

I think that this PR is ready to be merged. @pshriwise, do you agree?