-
Notifications
You must be signed in to change notification settings - Fork 0
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/65 unify hadrons with electrons #68
base: master
Are you sure you want to change the base?
Conversation
hadrons/common/continous_beam.py
Outdated
sqrt(x_chamber_dist_squared + (j - self.xy_middle_idx) ** 2) | ||
< self.inner_radius | ||
): | ||
# TODO: I'm not sure why this check is here - when separatioon_time_steps >= computation_time_steps, |
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.
@jbrage I've copied this if
from the cython implementation, but I'm not sure why it's here. It currently discards the initialized particles from the first few iterations - if any beams create charged particles during that time, they won't be accounted for in the result. Is this correct?
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.
@retkiewi yes, this actually is correct. The time it takes charged particles to drift half the electrode gap distance is an important characteristic time parameter here, because it is related to the steady-state case: Before that "separation time", there is a charge build-up giong on in the chamber. After that time, the charge in the ionization chamber is approximately constant, and we are generally only interested in the recombination at this steady-state level, i.e. the fraction of recombination after the "separation_time_steps".
So essentially, we should do the bookkeeping after "separation_time_steps", where we would count (i) the number of charge initialised, (ii), the charge collected, and calculate/count (iii) the fraction of charge recombination, i.e. the recombination factor
does that make sense?
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.
Yeah, that makes sense. Thanks! 😄
# number of beams - this ensures a uniform distribution of beams over the beam generation time | ||
# TODO: check if the 1st implementation bias was intentional or accidental | ||
|
||
# normalized_delays = summed_delays / summed_delays[-1] * beam_generation_time |
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.
@jbrage I've noticed that the beam distribution was biased towards the later bins since we normalize the times by the last value (so the last track would always be put in the last bin - it would be simulated right before the simulation time steps). The new code below should simulate an even distribution across all time steps (before separation).
This new even distribution can sometimes fall in the first few time steps and get discarded by the code I mentioned in the other comment. This results in the simulation not calculating the recombination correctly (since we didn't count any initialized charge carriers).
Should we bias the distribution towards the later time steps?
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.
@retkiewi you are right: the distribution should not be biased towards the later time steps, that is a mistake. The distribution should be even across all bins
Updates the hadron package to have implementations similar to the electron package