Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pulse height tally for photons #2452
Pulse height tally for photons #2452
Changes from 29 commits
974a720
dd18f12
199e23b
38b7f70
89820f2
c676ad7
eeb8a49
82f7b90
11a7ed2
7c2142e
3ce4b9c
7cdbada
7c9c991
01e3506
a746d37
158c3af
d32174e
09de63d
e401990
78bd621
deb4196
1712d5c
7090c5c
7c7c0a4
2626347
fcebe5c
74a1640
795145a
7516002
efc476f
6873cc4
91f561a
c34aa0c
32714aa
31e47b1
1e0365f
1d69270
9274af0
f42d316
1dcb51e
6d4cfee
27692b5
538a06e
d7423fb
d1a61fa
62f8c58
8ff651e
a727c4c
36172d3
464e504
8e260d9
ac2e6e2
4b9f2e1
11ec770
dbf6aa4
7967e8f
57f1425
6b7bcf0
d689d91
8fab57a
fb547a9
16cbc2a
7c0804a
48d2277
cfd219a
7aa7c4f
969ef10
63db229
a630e89
0bf609d
e8a412d
71b0e17
3eb998f
ff0f6db
e4b574d
6f7d8ad
01a03a6
c796fa1
b10a32e
b43ebae
24d8cb5
ebee016
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the particle is in a cell that is not listed in
pulse_height_cells
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats an important point! I will just call the action when the cell is listed:
if(it != model::pulse_height_cells.end()){
think this should solve the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it will change the behavior for
CellBornFilter
? I've always thought of cell born as corresponding to the cell that the original source particle was born in (not the cell a secondary particle was born in), although looking through the code now, I'm not sure that's actually how it's treated at present. Curious to hear your thoughts. Either way, setting thecell_born
attribute on a secondary particle probably belongs somewhere else.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion on this already in the old version of the pht:
https://github.com/openmc-dev/openmc/pull/1881/files#r726887201
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have simulated a simple example (happy to share) and currently the
cell_born
attribute is not corresponding for the original source particle. If a secondary particle is created in a cell thecell_born
attribute is set to this cell for the secondary particle.Maybe we should add a small comment into the docs?
Instead of
Bins tally events based on which cell the particle was born in.
maybe the following solves some confusion:
Bins tally events based on which cell the particle was born in. For secondary particles this attribute is set to the cell in which they were produced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how the
cell_born
attribute is currently handled in my understanding:None
entry.transport_history_based_single_particle
function we are calling in the first step of the loop always thep.event_calculate_xs()
method. In this method, thecell_born
attribute is set.In the case of the pulse-height tally we have to remove the energy of secondary particles that were created, cause their energy may not remain in the cell. So whenever a particle is created we remove the energy directly from the pulse_height value.
Since this attribute is set from a None to the cell the particle was born in only a bit earlier, this will not change the behaviour of the
CellBornFilter
.I see that this is not really elegant. But changing it would require a method to distinguish whether a particle is a primary or a secondary particle. Therefore I would recommend to leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry for my misunderstanding. Now I see that this code is actually exactly what's being called in
event_calculate_xs
, so no behavior is changing. That being said, it still feels awkward to me that this block of code is called inpht_secondary_particles
. Can you move it up intoevent_revive_from_secondary
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so of course it would be best if we could design the code in a way that the birth cell needs to be determined only in one place.
But, as said, I think this is hardly doable. Since not all particles start from
event_revive_from_secondary()
. So my conclusion from this is that we need to determine it separately for the secondary particles for the pulse-height tally.On the other hand, it is not necessary to determine the
birth_cell
for all secondary particles. We have to do this only if the information is needed for the pulse-height tally.Coming from that argumentation, I set the
birth_cell
now in the if condition just before the pulse-height function is called.So the birth cell is just determined before the
condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here -- seems like this could result in out-of-bounds access