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 momentum() function for psp #88

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

AntonReinhard
Copy link
Member

@AntonReinhard AntonReinhard commented Jul 5, 2024

Adds functions

momentum(psp::AbstractPhaseSpacePoint, dir::ParticleDirection, species::AbstractParticleType, n::Val{N})
momentum(psp::AbstractPhaseSpacePoint, dir::ParticleDirection, species::AbstractParticleType, n::Int)

to get the momentum of the n-th particle of the given species and direction from the AbstractPhaseSpacePoint. I provided two overloads: One with a compile-time constant N via Val{N} and one with a normal n::Int. The first one adds no overhead and is compiled away completely, but only if the given value is actually a compile-time constant, such as a literal 1.
The second overload is implemented separately and is faster than the first when n is not a compile-time constant. It still takes ~40ns on my machine, versus ~1ns when the first overload is used. The first overload with a dynamic value (Val(n) on variable n) takes ~100ns.

Currently I can't add automated unit tests here yet, since the tests are in QEDcore still.

@AntonReinhard AntonReinhard requested a review from szabo137 July 5, 2024 13:37
@AntonReinhard AntonReinhard added this to the Release-next milestone Jul 5, 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.

Looks good so far. Do you mind adding convenient defaults, e.g.
momentum(psp, dir,particle, v=Val(1)) if there is only one particle with (dir, particle) in the psp?

@AntonReinhard
Copy link
Member Author

Looks good so far. Do you mind adding convenient defaults, e.g. momentum(psp, dir,particle, v=Val(1)) if there is only one particle with (dir, particle) in the psp?

Good idea, I pushed it.

@AntonReinhard
Copy link
Member Author

Looks good so far. Do you mind adding convenient defaults, e.g. momentum(psp, dir,particle, v=Val(1)) if there is only one particle with (dir, particle) in the psp?

I just realized that with the number_particles implementation from #90 this might be possible much more easily. I'm not 100% sure if it would add overhead, but I'd wait and try that.

@AntonReinhard AntonReinhard marked this pull request as draft July 9, 2024 09:41
@AntonReinhard AntonReinhard marked this pull request as ready for review July 9, 2024 17:45
@AntonReinhard
Copy link
Member Author

Looks good so far. Do you mind adding convenient defaults, e.g. momentum(psp, dir,particle, v=Val(1)) if there is only one particle with (dir, particle) in the psp?

I just realized that with the number_particles implementation from #90 this might be possible much more easily. I'm not 100% sure if it would add overhead, but I'd wait and try that.

It does not add overhead :)

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've one last suggested change in the error message.

src/interfaces/phase_space_point.jl Outdated Show resolved Hide resolved
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.

Looks good for me. Thanks again for the implementation!

@szabo137 szabo137 merged commit 30d398b into QEDjl-project:dev Jul 10, 2024
4 checks passed
szabo137 pushed a commit that referenced this pull request Jul 11, 2024
Adds the process test from QEDcore to QEDbase in order to test the
implementations relying on the process interface.
Once #95, #90, and #88 are merged, I can add tests for their
functionalities here as well.
@AntonReinhard AntonReinhard deleted the momentum_function branch July 11, 2024 10:10
AntonReinhard added a commit to AntonReinhard/QEDbase.jl that referenced this pull request Jul 11, 2024
Adds functions 
```Julia
momentum(psp::AbstractPhaseSpacePoint, dir::ParticleDirection, species::AbstractParticleType, n::Val{N})
momentum(psp::AbstractPhaseSpacePoint, dir::ParticleDirection, species::AbstractParticleType, n::Int)
```
to get the momentum of the n-th particle of the given species and
direction from the `AbstractPhaseSpacePoint`. I provided two overloads:
One with a compile-time constant N via `Val{N}` and one with a normal
`n::Int`. The first one adds no overhead and is compiled away
completely, but only if the given value is actually a compile-time
constant, such as a literal `1`.
The second overload is implemented separately and is faster than the
first when `n` is not a compile-time constant. It still takes ~40ns on
my machine, versus ~1ns when the first overload is used. The first
overload with a dynamic value (`Val(n)` on variable n) takes ~100ns.

Currently I can't add automated unit tests here yet, since the tests are
in QEDcore still.
AntonReinhard added a commit to AntonReinhard/QEDbase.jl that referenced this pull request Jul 11, 2024
Adds the process test from QEDcore to QEDbase in order to test the
implementations relying on the process interface.
Once QEDjl-project#95, QEDjl-project#90, and QEDjl-project#88 are merged, I can add tests for their
functionalities here as well.
AntonReinhard added a commit to AntonReinhard/QEDbase.jl that referenced this pull request Jul 11, 2024
Adds functions 
```Julia
momentum(psp::AbstractPhaseSpacePoint, dir::ParticleDirection, species::AbstractParticleType, n::Val{N})
momentum(psp::AbstractPhaseSpacePoint, dir::ParticleDirection, species::AbstractParticleType, n::Int)
```
to get the momentum of the n-th particle of the given species and
direction from the `AbstractPhaseSpacePoint`. I provided two overloads:
One with a compile-time constant N via `Val{N}` and one with a normal
`n::Int`. The first one adds no overhead and is compiled away
completely, but only if the given value is actually a compile-time
constant, such as a literal `1`.
The second overload is implemented separately and is faster than the
first when `n` is not a compile-time constant. It still takes ~40ns on
my machine, versus ~1ns when the first overload is used. The first
overload with a dynamic value (`Val(n)` on variable n) takes ~100ns.

Currently I can't add automated unit tests here yet, since the tests are
in QEDcore still.
AntonReinhard added a commit to AntonReinhard/QEDbase.jl that referenced this pull request Jul 11, 2024
Adds the process test from QEDcore to QEDbase in order to test the
implementations relying on the process interface.
Once QEDjl-project#95, QEDjl-project#90, and QEDjl-project#88 are merged, I can add tests for their
functionalities here as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants