Fix for Weight Window Scaling Bug#3511
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug in the weight window scaling logic where the survival weight was not being scaled along with the lower and upper weight bounds. This caused particles to always pass roulette checks with significantly reduced weights, leading to substantial bias in tallies (e.g., JET problem showing 1.45e-09 vs expected ~2.8e-09 flux values).
- Adds scaling of survival_weight in the WeightWindow scale() method
- Ensures consistent weight window behavior during rouletting/splitting operations
- Addresses significant tally bias observed in complex geometries like JET tokamak simulations
|
|
||
| def test_weightwindows(model): | ||
| test = HashedPyAPITestHarness('statepoint.2.h5', model) | ||
| test = TolerantPyAPITestHarness('statepoint.2.h5', model) |
There was a problem hiding this comment.
Thanks for updating the results! Why the change to the tolerance test harness? The results should still be reproducible right?
There was a problem hiding this comment.
I wasn't able to reproduce the statepoint hash even for that test on the original develop branch (I tried on both my mac and a linux cluster). Even for serial execution, compilers have a free hand in re-ordering operations that are associative mathematically, but non-associative for floating point, such that the bit-wise reproducibility that statepoint hashing requires seems difficult to achieve in a portable manner. The tolerant test harness still requires the simulation be reproducible, but results are rounded such that everything doesn't need to be bitwise the same.
That said, I'm surprised it is still failing now with the tolerant test. The test passes on both my mac and the linux cluster I'm on (with gcc 11.1, 13.3, and LLVM 20.1.2), so I'm not sure what is causing the failure. Will look into it a bit more, perhaps we need to run with fewer particles or something.
|
Update: I converged things a little more for both the analog and new weight window runs in OpenMC, and also looked at the B1 results since they were more well resolved with the higher runtime in the analog case. The results show very close agreement between OpenMC (with and without weight windows) and MCNP (which was converged to +/-5%). Thus, I don't believe there is any remaining bias due to weight windows -- this PR should fix everything.
Now I just need to figure out the issue with the testing... |
paulromano
left a comment
paulromano
left a comment
There was a problem hiding this comment.
@jtramm Thanks for this important fix! When I update the test result on my machine, it looks like the hash value matches what gets produced on our CI runners so I expect the tests should pass now 🟢 Not sure why your system is producing different values but I wouldn't be surprised if it comes down to some subtle difference between Linux and Mac (part of the reason we still don't have CI configurations on macOS).
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
Description
This PR fixes a bug in the weight window scaling logic. Currently, weight windows are scaled up/down in two areas in response to certain conditions being met. In the case of birth weight scaling (PR #3459), this is done to ensure that weight windows are scaled so as to be responsive to the weight window value of where the particle was sampled, such that rouletting/splitting does not happen immediately at birth.
I was noticing some significant bias in tallies in the JET problem when weight windows were enabled vs. just analog MC. This was being caused by the scaling function only scaling the lower and upper weight window bounds, rather than the survival weight as well. In effect, when particles would be in a weight window zone that triggered the rouletting routine, the routine would have a survival weight that would be far below the actual particle weight. This means the particles would always pass the roulette successfully and have their weights cut down to the unscaled survival weight, leading to a significant overall net loss of weight from the simulation.
This fix simply ensures the survival weight also gets scaled, such that rouletting happens in the expected manner.
Below is the data for a specific detector tally in JET (the A1 TLD which is quite close to the tokamak, allowing for it to be possible to get some tallies scores on with just analog MC).
Notably, the difference between the analog and WW cases is still larger than expected. There is a lot of uncertainty with the analog case, however, so it's unclear if this is real bias or just noise. Even if there is another bug in the weight window logic somewhere else still causing bias, the current fix is still necessary.
Checklist