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

Add PhaseSpacePoint definition #51

Merged
merged 12 commits into from
May 7, 2024

Conversation

AntonReinhard
Copy link
Member

Proposal to fix issue #46

This branch is currently based on PR #39 and should be merged after it.

This is a draft for now, but I think it is a good compromise from the discussion in issue #46. The definition is type stable, allows some nice overloads for is_fermion and so on for the ParticleStateful objects, and takes care of preventing illegal objects in the constructors.

Creating particles is straightforward and disallows illegal configurations (electron with polarization or photon with spin):

in_el = ParticleStateful(rand(SFourMomentum), Electron(), AllSpin(), Incoming())
in_ph = ParticleStateful(rand(SFourMomentum), Photon(), AllPol(), Incoming())
out_el = ParticleStateful(rand(SFourMomentum), Electron(), AllSpin(), Outgoing())
out_ph = ParticleStateful(rand(SFourMomentum), Photon(), AllPol(), Outgoing())

in_particles_valid = SVector(in_el, in_ph)
in_particles_invalid = SVector(in_el, out_ph)

out_particles_valid = SVector(out_el, out_ph)
out_particles_invalid = SVector(out_el, in_ph)

Creating PhaseSpacePoints is similarly simple and prevents illegal configurations, e.g. incoming particles in the outgoing list, but is still reasonably fast and does not need allocations. One PhaseSpacePoint for a 2 to 2 process is 944B in memory.

julia> @benchmark PhaseSpacePoint($process, $model, $phasespace_def, $in_particles_valid, $out_particles_valid)
BenchmarkTools.Trial: 10000 samples with 991 evaluations.
 Range (min  max):  39.268 ns  111.740 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     41.068 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   43.116 ns ±   6.557 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

   ▃▇█▆▂ ▁▂▁▁                                             ▁    ▁
  ▆███████████▇▆▆▇▆▆▆▅▅▆▆▆▅▄▅▅▄▄▆▆▆▆▆▆▇▇▆▇▇█▇▅▄▅▅▄▅▄▅▄▅▆████▇▇ █
  39.3 ns       Histogram: log(frequency) by time      70.9 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

Suggestions welcome

Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

Thank you for this!

This is not a full review of the changes, but some first obvious comments from my side.

Can you explain, why the unit tests for Julia v1.6 and v1.7 fail, but the others not? Looks a bit like an API change for @test_throws.

test/phase_spaces.jl Outdated Show resolved Hide resolved
test/phase_spaces.jl Outdated Show resolved Hide resolved
test/phase_spaces.jl Outdated Show resolved Hide resolved
@AntonReinhard
Copy link
Member Author

Can you explain, why the unit tests for Julia v1.6 and v1.7 fail, but the others not? Looks a bit like an API change for @test_throws.

Yeah I'm pretty sure that's what it is... I think the matching to a regex is new. I can just wrap it in a check about the Julia version, but are we sure we need to support 1.6 and 1.7 anyways? Especially when the now stable version is 1.10 already.

@AntonReinhard AntonReinhard force-pushed the phase_space_point branch 2 times, most recently from 87006a8 to 6f7cb5e Compare May 3, 2024 19:49
@AntonReinhard AntonReinhard self-assigned this May 3, 2024
@szabo137 szabo137 added this to the Release-next milestone May 3, 2024
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

I like the implementation, here are some comments from my side.

Furthermore, we should think about some custom Base.show methods for these nested types.

src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
@szabo137
Copy link
Member

szabo137 commented May 4, 2024

Can you explain, why the unit tests for Julia v1.6 and v1.7 fail, but the others not? Looks a bit like an API change for @test_throws.

Yeah I'm pretty sure that's what it is... I think the matching to a regex is new. I can just wrap it in a check about the Julia version, but are we sure we need to support 1.6 and 1.7 anyways? Especially when the now stable version is 1.10 already.

Julia v1.6 is the defacto LTS, but you are right, the tests for v1.7 to v1.9 are somewhat arbitrary. However, I am fine with the exclusion of some tests, if there is an API change of lack of support in the respective Julia version. Therefore, there is no need to drop the support in the current status, but we should keep an eye on that.

Regarding the support of Julia v1.10, you are right, of course. There should be an issue opened on its inclusion 😉

src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
@SimeonEhrig SimeonEhrig removed their request for review May 6, 2024 11:43
@AntonReinhard AntonReinhard marked this pull request as ready for review May 6, 2024 17:32
szabo137
szabo137 previously approved these changes May 6, 2024
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

Just some minor issues. Can be merged if resolved.

src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
Co-authored-by: Uwe Hernandez Acosta <[email protected]>
@szabo137
Copy link
Member

szabo137 commented May 6, 2024

Btw: we must not set assignees on pull requests, because it breaks the gitlab bot somehow 🤷‍♂️

@szabo137 szabo137 removed the request for review from steindev May 6, 2024 20:07
@AntonReinhard
Copy link
Member Author

Oh you're right, I completely forgot

Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

Since there is no open issue for this one, I am happy to merge!

@szabo137 szabo137 merged commit 9af1887 into QEDjl-project:dev May 7, 2024
4 checks passed
@AntonReinhard AntonReinhard deleted the phase_space_point branch May 7, 2024 13:11
szabo137 added a commit that referenced this pull request May 14, 2024
With this PR, we adopt the phase space points from #51 for the
probability and cross section interface.

Further changes: we drop the support of different phase space
definitions for the in- and out-phase space.

⚠️ This is based on
https://github.com/AntonReinhard/QEDprocesses.jl/tree/phase_space_point
and should be rebased to `dev` and considered for merging *after* #51 is
merged.

---------

Co-authored-by: Anton Reinhard <[email protected]>
Co-authored-by: Uwe Hernandez Acosta <[email protected]>
@szabo137 szabo137 added 06 - Feature-request Missing a feature or functionality moved-to-QEDcore labels Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
06 - Feature-request Missing a feature or functionality moved-to-QEDcore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants