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

Feature/northroj/time cell tallies #282

Merged

Conversation

northroj
Copy link
Contributor

This PR adds time and energy binning to cell tallies Ethan added recently.

The structure was primarily copied from the mesh tallies. Domain decomposition support is currently not included. It is possible/probable that cell tallies crossing domain decomposition borders will cause errors, but it is unlikely that cell tallies would be used in the types of large problems where domain decomposition is used.

This has been tested on an approximate pulsed sphere problem currently being added to the challenge problem suite in conjunction with time census.

@northroj northroj added the enhancement New feature or request label Jan 27, 2025
@northroj
Copy link
Contributor Author

I also updated the coincidence tolerance in this PR. I have not yet run into a geometry which causes this tolerance to fail, but I will keep looking.

1 similar comment
@northroj
Copy link
Contributor Author

I also updated the coincidence tolerance in this PR. I have not yet run into a geometry which causes this tolerance to fail, but I will keep looking.

@ilhamv ilhamv self-requested a review January 29, 2025 09:46
@ilhamv
Copy link
Member

ilhamv commented Jan 29, 2025

Thanks, Jordan!

There are possible cases in which one would need a cell tally and domain decomposition at the same time. However, the cell tallies are less likely to be the kind of tally that needs decomposition. So, for now, this should be OK in most cases.

I plan to review the PR soon tomorrow.

@ilhamv
Copy link
Member

ilhamv commented Jan 29, 2025

It was weird that the MPI-Numba test passed, but the MPI-Python didn't. Re-running the latter now.

@ilhamv
Copy link
Member

ilhamv commented Jan 30, 2025

@northroj - can you add a regression test for the time-meshed cell tally feature?

@ilhamv
Copy link
Member

ilhamv commented Jan 30, 2025

@northroj - I really like your clear and complete input parameter descriptions in def cell_tally(). Can you update the other tally parameter descriptions? Thanks.

@northroj
Copy link
Contributor Author

northroj commented Feb 4, 2025

@northroj - I really like your clear and complete input parameter descriptions in def cell_tally(). Can you update the other tally parameter descriptions? Thanks.

I am not sure what this means. The docstring in def cell_tally() was copied from the other tally docstrings, and I don't know what other tally parameter descriptions to update.

@northroj
Copy link
Contributor Author

northroj commented Feb 4, 2025

I just realized I cant use continuous energy for the regression test, I will get a mg one in.

@northroj
Copy link
Contributor Author

northroj commented Feb 4, 2025

For @ilhamv: For a test of the cell tallies, I just added a cell tally to the inf_shem361 problem along with census to test the time and energy binning along with time census. I had to comment out the mesh tally because it produced a NAN for the stdev in the final energy group bin when census was enabled (it does not show up when census is commented out). I will add this to an issue.

@ilhamv
Copy link
Member

ilhamv commented Feb 6, 2025

Oops. Looks like I messed up the commit history... Let me try fixing it.

@ilhamv ilhamv force-pushed the feature/northroj/time_cell_tallies branch from 6dfa7c5 to e1fa196 Compare February 6, 2025 01:51
@ilhamv ilhamv merged commit 0b38e2d into CEMeNT-PSAAP:dev Feb 6, 2025
10 of 14 checks passed
@clemekay clemekay added this to the v0.12.0 milestone Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants