Fixed a bug in cell last update

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.

[GuySten]

Description

Currently, cell_last is assigned only in surface crossing events.
This PR fix that and now this data is set also after collision.

EDIT:
This PR also fix material_last update to make CellFromFilter and MaterialFromFilter consistent.

@paulromano, can you look into this?
This is a subtle change and I am not 100 percent sure this fix the problem.
This issue does change regression tests.

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 18) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• 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)

[paulromano]

The cell_last change alters the physical meaning of volumetric tallies with CellFromFilter. Currently, using CellFromFilter for a volumetric tally in cell B answered: "How much of the score in B comes from particles that entered B from cell A?" To me, this is a physically meaningful decomposition of scores by source cell. With the PR, cell_last always equals the current cell, so CellFromFilter for volumetric tallies no longer provides "from where" information as far as I can tell. My opinion is that we should not change the current behavior.

That being said, the fix for material_last in event_cross_surface is a legitimate bug fix so I think we should trim the PR down to just that change.

[GuySten]

Maybe I missed something but I think that if we only change cell last in surface crossing then when a particle cross from cell A to cell B cell last is cell A and everything is OK.
But currently if afterwards the particle collide in cell B multiple times then the cell last variable is still cell A because we did not cross any surface and did not update cell last.

Maybe I broke cell last but @paulromano do you at least agree that the current behavior of cell last is a bug?

[GuySten]

Actually I did break cell last so I've added another unit test and edited the code until it passes.

[GuySten]

Some notes:
The behavior of the code is made complicated by the fact that openmc know to recalculate macro xs by setting material_last to C_NONE.
To distinguish between material void I had to change its id to -2.

[GuySten]

This PR will need some serious digging.

[GuySten]

I tried to make all the tests pass at the cost of always recalculating cross sections.
That attempt can be found here #3858 .
It almost succeeded (except one dagmc test).

I understand that the cost of always recalculating xs is too much.
@paulromano, can you help figure out what is going on?
Anytime I think I fixed the problem by fixing one test I break something else...

[JoffreyDorville]

I have been working on this part of the code several months ago and I agree with @paulromano on what we expect for the cell_last behavior. I see this attribute as a way of tracking the cell where the particle was before the last surface crossing.

@GuySten If I understand correctly, you expect cell_last to update when a collision occurs or when a surface is crossed. What would be the application for such behavior? Do you have a particular tally process in mind?

[GuySten]

@JoffreyDorville, you understood me correctly.
I think that because every other attribute (E_last, r_last, ...) are updated after collision then the fact that cell isn't is unexpected.
This affects reaction tallies when using cellfromfilter.
I discovered this difference when I tried implementing forced collision.
I wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened.
I think the current behavior is weird.

[GuySten]

If the meaning of cell last and material last really should be the last cell and material before entering the current cell this should at least be better documented.

[GuySten]

@JoffreyDorville, I also noticed that your interpretation does not agree with one of the regression tests:
In test_cellfrom there is a check:

# Comparison to global from 3 assert_allclose(t3_3 + t3_4 + t4_3 + t4_4, tglobal, rtol=RTOL, atol=ATOL)

That the total reaction rate is the sum of the reaction rate from 3 to 3 from 3 to 4 from 4 to 4 and from 4 to 3.
In your interpretation from 3 to 3 and from 4 to 4 should be zero and because some particles start in 3 or 4 and interact there this check cannot pass because it cannot equal the total reaction rate.

[GuySten]

I think a decision should be made about what the meaning of cell_last and material_last should be and then the relevant tests should be changed accordingly.

@paulromano, what do you think?

[vitmog]

I would like to clarify the origin of the topic, which is seemingly related to a splitting scheme:

wanted to stop splitting the history when cell last is equal the current cell and I discovered that this never happened

@GuySten What do you need to stop it for? Currently, you have a perfect chance to apply splitting more than one time to the same particle both before and after iteraction sampling. It can be valuable if you are able to know the splitting rate. If you skip splitting then, you will implement a worst scheme.

[GuySten]

@vitmog, My understanding is that when a particle enter into a forced collision cell we split it into an uncollided history and collided history. If we apply forced collision again to the collided history we might enter a very long loop so I split only the original particle.

[vitmog]

@GuySten You are right, the original MCNP forced collision (FC) scheme requires splitting once per cell visit only. The variant that I had in mind necessarily requires combining it with weight windows (WW) or an equivalent as a source of the importance/splitting rate. However, nothing prevents to combine both FC and WW on the user side.