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

Refactoring of differential cross section functionality #38

Merged
merged 29 commits into from
Mar 7, 2024

Conversation

szabo137
Copy link
Member

@szabo137 szabo137 commented Nov 14, 2023

Problem statement

The current implementation only allows implementing differential_cross_section directly for given processes. Furthermore, the interface does not allow the implementation of different phase-space coordinate systems, e.g. to model $d\sigma/dE$ and $d\sigma/d\cos\theta$ for the same process.

Suggested solution

Advanced process interface

I suggest to disassemble the differential cross-sections interface into a more flexible interface:

flowchart TD
  _differential_cross_section --> _differential_probability
  _differential_cross_section --> _incident_flux*
  _differential_probability --> _avg_matrix_element_square
  _differential_probability --> _phase_space_factor*
  _differential_probability -.-> _is_in_phasespace*
  _avg_matrix_element_square --> _matrix_element*
  _avg_matrix_element_square --> _avg_normalization*
Loading

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 elements

The 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 versions differential_cross_section and differential_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:

PhasespaceDefinition{
    CS<:AbstractCoordinateSystem,
    F<: AbstractFrameOfReference
}

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

_is_in_phasespace(
    in_ps_def::AbstractPhasespaceDefinition,
    in_ps::AbstractVector{T},
    out_ps_def::AbstractPhasespaceDefinition,
    out_ps::AbstractVector{T},
) where {T<:QEDbase.AbstractFourMomentum}

which returns true if the input is physical, and false 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

_unsafe_differential_cross_section # without input validation, without phase-space check
_differential_cross_section        # without input validation, with phase-space check
unsafe_differential_cross_section  # with input validation, without phase-space check
differential_cross_section         # with input validation, with phase-space check

_unsafe_differential_probability   # without input validation, without phase-space check
_differential_probability          # without input validation, with phase-space check
unsafe_differential_probability    # with input validation, without phase-space check
differential_probability           # with input validation, with phase-space check

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:

  • write tests for (_(unsafe_))probability
  • make total cross-section an interface function
  • make energy-momentum check for safe implementations an interface function
  • Write a gist about the broadcasted implementations
  • rename _[unsafe_]probability to _[unsafe]differential_probability
  • cleanup in Project.toml
  • open issue on PhasespaceDefinition

Copy link
Member

@AntonReinhard AntonReinhard left a 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.

src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/cross_sections.jl Outdated Show resolved Hide resolved
src/cross_sections.jl Outdated Show resolved Hide resolved
@AntonReinhard
Copy link
Member

Another thought: We should probably add an @inline to all of the functions that just delegate by broadcast or some check.

@szabo137 szabo137 marked this pull request as ready for review February 1, 2024 09:09
@szabo137 szabo137 requested a review from steindev February 1, 2024 09:19
Copy link
Member

@steindev steindev left a 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.

src/QEDprocesses.jl Outdated Show resolved Hide resolved
src/cross_sections.jl Show resolved Hide resolved
src/cross_sections.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
test/cross_sections.jl Outdated Show resolved Hide resolved
test/utils/groundtruths.jl Outdated Show resolved Hide resolved
test/cross_sections.jl Outdated Show resolved Hide resolved
test/utils/random_momenta.jl Outdated Show resolved Hide resolved
test/utils/random_momenta.jl Outdated Show resolved Hide resolved
@szabo137
Copy link
Member Author

@steindev Thanks for reviewing, I hopefully addressed all comments. Ready for revisiting!

Copy link
Member

@tjungni tjungni left a 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

src/cross_sections.jl Outdated Show resolved Hide resolved
src/cross_sections.jl Outdated Show resolved Hide resolved
src/cross_sections.jl Show resolved Hide resolved
src/cross_sections.jl Outdated Show resolved Hide resolved
src/cross_sections.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
steindev
steindev previously approved these changes Mar 6, 2024
Copy link
Member

@steindev steindev left a comment

Choose a reason for hiding this comment

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

All comments resolved. 💪

@steindev
Copy link
Member

steindev commented Mar 6, 2024

@tjungni would you please approve in case all your comments are resolved?

Copy link
Member

@tjungni tjungni left a 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.

test/interfaces/setup_interface.jl Show resolved Hide resolved
test/cross_sections.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
test/test_implementation/random_momenta.jl Outdated Show resolved Hide resolved
test/cross_sections.jl Outdated Show resolved Hide resolved
test/test_implementation/groundtruths.jl Outdated Show resolved Hide resolved
test/interfaces/process_interface.jl Show resolved Hide resolved
test/interfaces/process_interface.jl Show resolved Hide resolved
@szabo137
Copy link
Member Author

szabo137 commented Mar 7, 2024

@tjungni I addressed all of your comments and appreciate your approval.

Copy link
Member

@tjungni tjungni left a 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!

@szabo137 szabo137 merged commit f703634 into QEDjl-project:dev Mar 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
05 - Enhancement Improvements of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants