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

Proposal for PhaseSpacePoint improvement #63

Merged
merged 24 commits into from
May 22, 2024

Conversation

AntonReinhard
Copy link
Member

@AntonReinhard AntonReinhard commented May 19, 2024

Proposal for a rework of the PhaseSpacePoint, according to Issue #58

I did a lot of type magic with recursive variadic templates to find out type information and get functions perfectly type stable. Constructing phase space points is now always type stable and takes a maximum of ~11ns for me when constructing from momenta.

Some of the tests are failing for now because some of the interfaces currently don't expect tuples from the PSP implementation. It might make sense to fix this in #59

@AntonReinhard AntonReinhard marked this pull request as ready for review May 20, 2024 10:40
@AntonReinhard
Copy link
Member Author

I just noticed I'm still missing the new implementation of spin(...) / polarization(...) of a PSP. I'll add this later

@szabo137
Copy link
Member

@AntonReinhard Seems like something broke with this one. Maybe it is better to wait with this one until #59 is merged. After that, a rebase of this one seems easy.

@AntonReinhard
Copy link
Member Author

Should be building fine for now @szabo137

I'm not sure if it might actually be a good idea to merge this before #59 instead of the other way around, since there are slight changes in how the PSP behaves. For example, I added a momenta(::PSP, ::DIR) function to get an SVector of the momenta, because momentum.(psp.in_particles) now returns a tuple and not a vector. It doesn't do any copies for this and only takes about 5ns on my machine for a 2-length vector but it might still not be the best approach.

Also, I noticed that there is no public interface for a process' spins/polarizations, which means it's now impossible to get a particle's spin.

@AntonReinhard
Copy link
Member Author

I have tentatively merged #59 into this, but keeping it local for now. I'm playing around with it now and am noticing that the broadcast in momenta(...) prevents everything from working on the GPU (both ROCm and CUDA).

@AntonReinhard
Copy link
Member Author

@szabo137 Should be ready to merge now

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.

Excellent work! Only some minor comments.

src/QEDprocesses.jl Outdated Show resolved Hide resolved
src/phase_spaces.jl Outdated Show resolved Hide resolved
test/cross_sections.jl Show resolved Hide resolved
test/phase_spaces.jl 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.

LGTM

@szabo137 szabo137 merged commit 80dbcc2 into QEDjl-project:dev May 22, 2024
4 checks passed
@AntonReinhard AntonReinhard deleted the 58-ps-improve branch May 22, 2024 07:38
@szabo137 szabo137 added this to the Release-next milestone May 22, 2024
@szabo137 szabo137 mentioned this pull request May 27, 2024
@szabo137 szabo137 mentioned this pull request Jun 18, 2024
@szabo137 szabo137 added 04 - Performance Improvements or issues regarding performance and timing 05 - Enhancement Improvements of existing code labels Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
04 - Performance Improvements or issues regarding performance and timing 05 - Enhancement Improvements of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants