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

Point Detector Tally #3109

Open
wants to merge 85 commits into
base: develop
Choose a base branch
from
Open

Point Detector Tally #3109

wants to merge 85 commits into from

Conversation

itay-space
Copy link

@itay-space itay-space commented Aug 6, 2024

Description

This repository contains the development and benchmarking of a relativistic point detector within the OpenMC framework. The detector efficiently estimates neutron flux at a point using full relativistic kinematic calculations, making it ideal for high-energy particle simulations. This first implementation of a point detector tally in OpenMC is validated through single and multiple collision experiments, showing excellent agreement with conventional methods. This enhancement significantly improves OpenMC's capabilities for complex shielding analyses and other scenarios where traditional Monte Carlo methods may be inefficient.

Use Example

To configure a point tally in OpenMC, follow the standard procedure for creating a tally but specify the estimator as "point" and provide the coordinates for positioning and the exclusion sphere radius $R_0$ using a list of x, y, z, $R_0$ values. The following code snippet demonstrates how to set up a point detector located at coordinates (0, 0, 1) with an exclusion sphere radius of 0.01 cm, along with an energy filter, using the OpenMC Python API:

point_detector = openmc.Tally(name="pDet")
point_detector.scores = ["flux"]  
point_detector.filters = [energy_filter]  
point_detector.estimator = 'point'  
point_detector.positions = [0, 0, 1 , 0.01]  

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) 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)

Itay-max and others added 30 commits October 25, 2022 13:42
…g them, there energ was nan - ehat caused the memory problem
…nction. Works dynically with the number of soultions. verified.
@itay-space itay-space closed this Aug 6, 2024
@itay-space itay-space reopened this Aug 6, 2024
@itay-space itay-space changed the title Deploy Point Detector Tally Aug 6, 2024
@itay-space itay-space requested a review from shimwell as a code owner November 26, 2024 08:48
Copy link
Contributor

@gridley gridley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, there's a lot of new stuff going on here. My main question is: why do you think that point estimates should be interpreted as an estimating method for tallies in general? It seems like in fact, this is a special estimator that can only be applied for the point estimate filter. This special filter can only allow one type of estimator, the point.

Also, there is a substantial amount of undocumented code here, and no accompanying commentary on what's being done here in the PR documentation or docs changes. This is a nontrivial feature, so would need to be described a bit more. For example, I'm unsure what this is talking about when you say this is relativistic.

There is a fair amount of commented out debug printing. That would have to be removed before merging.

There must also be some limitations to this: for example, is this correct for treating resonance upscatter with DBRC or RVS? Getting the correctly normalized PDF value there is kind of nontrivial.

.clang-format Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@@ -281,7 +281,7 @@ enum class TallyResult { VALUE, SUM, SUM_SQ, SIZE };

enum class TallyType { VOLUME, MESH_SURFACE, SURFACE, PULSE_HEIGHT };

enum class TallyEstimator { ANALOG, TRACKLENGTH, COLLISION };
enum class TallyEstimator { ANALOG, TRACKLENGTH, COLLISION, POINT };
Copy link
Contributor

@gridley gridley Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really make sense as a way to architect this feature. Semantically, the estimate of a quantity at a point is an entirely different problem from the different approaches to estimating otherwise. To be specific, in this case you are calculating contributions to a tally outside the usual process of filtering the state of a particle.

IMO, this should be classified as an entirely different type of tally similar to how pulse height tallies were done. POINT should not be an option in TallyEstimator, since this would allow someone to make a mesh tally that has the POINT estimator type. That doesn't really make sense, if you ask me. This estimator is fundamentally tied to one specific type of filter, a (set of) dirac delta(s) in space.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We debated whether to implement this method as a filter or an estimator. Ultimately, we decided on using an estimator because we propose here a different method of estimating flux in space. However, we recognise that this decision might not be optimal given our limited familiarity with the overall structure of OpenMC. We would greatly appreciate your input on this matter.

include/openmc/distribution.h Show resolved Hide resolved
include/openmc/particle.h Outdated Show resolved Hide resolved
include/openmc/tallies/tally_scoring.h Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@itay-space
Copy link
Author

itay-space commented Jan 2, 2025

Wow, there's a lot of new stuff going on here. My main question is: why do you think that point estimates should be interpreted as an estimating method for tallies in general? It seems like in fact, this is a special estimator that can only be applied for the point estimate filter. This special filter can only allow one type of estimator, the point.

Also, there is a substantial amount of undocumented code here, and no accompanying commentary on what's being done here in the PR documentation or docs changes. This is a nontrivial feature, so would need to be described a bit more. For example, I'm unsure what this is talking about when you say this is relativistic.

There is a fair amount of commented out debug printing. That would have to be removed before merging.

There must also be some limitations to this: for example, is this correct for treating resonance upscatter with DBRC or RVS? Getting the correctly normalized PDF value there is kind of nontrivial.

Hello @gridley ,

Thank you for your thoughtful review and feedback on the code. We have added partial documentation to some of the functions and are actively working on documenting the rest. Regarding your comment about relativistic computations, the added documentation should make this aspect clear now. Please let us know if anything is still unclear or needs further elaboration.

We have recently submitted a paper to the Annals of Nuclear Energy detailing the development and benchmarking of the code.

Regarding your question about velocity sampling: we introduced v_t, which represents the most recently sampled target velocity within the particle data class. This allows us to calculate the angular probability density function (PDF) for scattering towards the detector based on a specific sampled velocity.

Thank you again for your time and guidance.

Best regards,
Itay

@itay-space itay-space requested a review from gridley January 4, 2025 22:57
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.

4 participants