-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactoring of differential cross section functionality #38
Conversation
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.
Ok so if I understand correctly, the "pipeline" is as following:
- Public safe crossdiff [-> broadcast] -> Private safe crossdiff -> Private unsafe crossdiff -> Private unsafe probability
- Public unsafe crossdiff [-> broadcast] -> Private unsafe crossdiff -> Private unsafe probability
- Public safe probability [-> broadcast] -> Private safe probability -> Private unsafe probability
- Public unsafe probability [-> broadcast] -> Private unsafe probability
I like that the broadcast happens first thing, though it also means that for all 4 implementations we need 3 extra functions. Maybe we could reduce that to 2:
- one implementation that takes both in and out_phasespace as
AbstractVecOrMat
- one implementation that takes in_phasespace as
AbstractVecOrMat
and out as only Vec
The first one would then broadcast only over the out_phasespace.
With julia's dynamic threaded option this shouldn't be a problem with nested threaded loops. Only problem could be with broadcasting on the GPU maybe.
Another thought: We should probably add an |
035a101
to
a844685
Compare
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.
As expected, only a couple of minor comments.
@steindev Thanks for reviewing, I hopefully addressed all comments. Ready for revisiting! |
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.
First half of review
Co-authored-by: Tom Jungnickel <140055258+tjungni@users.noreply.github.com>
…-fail rand momenta
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.
All comments resolved. 💪
@tjungni would you please approve in case all your comments are resolved? |
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.
Added some final comments.
@tjungni I addressed all of your comments and appreciate your approval. |
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.
Everything looks good, ready to merge!
Problem statement
The current implementation only allows implementing$d\sigma/dE$ and $d\sigma/d\cos\theta$ for the same process.
differential_cross_section
directly for given processes. Furthermore, the interface does not allow the implementation of different phase-space coordinate systems, e.g. to modelSuggested solution
Advanced process interface
I suggest to disassemble the differential cross-sections interface into a more flexible interface:
where the functions with the asterisk give the new interface for the calculation of differential cross-sections:
_matrix_element
: the matrix element of the process_phase_space_factor
: the value of the pre-differential factor of the phase-space measure_is_in_phasespace
: checks if a given input is physical, see below_incident_flux
: the incident particle flux, used to normalize the probability_avg_normalization
: normalization for averaging of squared matrix elementsThe underscore of those functions implies, that they are not exported, and no input validation check is performed. For the functions
_differential_cross_section
and_differential_probability
there are versionsdifferential_cross_section
anddifferential_probability
, where input validation is performed.Phase space definition
To attack the original problem using different coordinate systems for the same process, this PR adds a simple system for coordinate systems and frames of reference. Those are propagated through a composite type:
Currently, the given example implementations are very limited, but this deserves an interface in the future. A dedicated issue will be opened during the review process of this PR (see todos below).
Phase space check
Not included in the input validation is the check if given incoming and outgoing phase-spaces are physical, i.e. if all momenta are on-shell and fulfill some sort of energy-momentum conservation. Therefore, I suggest to add another interface function
which returns
true
if the input is physical, andfalse
otherwise. Consequently, there are unsafe versions of cross-sections and probabilities, which don't perform the check given by the function above, and safe versions, which return null if the condition given by_is_in_phasespace
is not met.Final remarks
To conclude, the different versions of cross-section and probability functions, derived from the interface above, are
where only the functions without an underscore are exported, for all of those functions, there are implementations for all combinations of vectors and matrices for the incoming and outgoing phase space.
Todos:
(_(unsafe_))probability
_[unsafe_]probability
to_[unsafe]differential_probability
PhasespaceDefinition