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

Automated time dependant weight windows #255

Draft
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

shac170
Copy link
Contributor

@shac170 shac170 commented Nov 11, 2024

Added functionality for automated time dependant weight windows based on the previous timestep solution.

@ilhamv
Copy link
Member

ilhamv commented Nov 15, 2024

@shac170, I edited the regression test so that N_batch > 1 so that stdev is estimated via batch statistics, which is the proper way to do for simulation with census. This also fixes the failing test. I also reduced N_particle and number of census grids to make the test runs faster.

@ilhamv
Copy link
Member

ilhamv commented Nov 16, 2024

@shac170 , I put comments and rearranged the update_weight_window. Note that those are just suggestions, please go ahead revert it back or make more edits if needed.

@ilhamv
Copy link
Member

ilhamv commented Nov 17, 2024

The previously failing Numba test is due to the use of a boolean array for indexing in searching for the minimum flux in the weight window update function. I rewrite it into the explicit for-loops version. It is weird that it passed before the rearrangement was made.

The MPI test should still fail. I think it is because, in the multi-batch mode, we only reduce the tallies, including the flux, at the end of each batch, while with the automated WW, we need to reduce it at the end of each census.

My suggestion is that instead of using the MC/DC tally to store WW flux, we should allocate a dedicated WW flux array. Let me know that you think, @shac170 .

@shac170
Copy link
Contributor Author

shac170 commented Nov 18, 2024

The modifications to avoid boolean array indexing and all other modifications look good to me. I understand the issue with only reducing tallies at the end of the batch. Would it be too expensive to reduce it at each census? I am also confused on what a separate WW tally would look like (and the additional cost associated with it) @ilhamv

@ilhamv
Copy link
Member

ilhamv commented Nov 19, 2024

@shac170 , I think it is inevitable that we need to reduce at each census as all processes need to know the new updated window...
The WW flux tally would be of the same shape as the window itself, and that WW flux tally is the only thing that we need to reduce at each census. Note that this is also needed for flexibility. For example, a user quantity of interest may be just a single cell tally, not flux distribution across the domain; in this case, a separate dedicated WW flux tally will be needed. The two types of tallies serve different functions: one is for WW purposes, and the other one is for the problem's actual quantity of interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants