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

Update process interface interface #59

Merged
merged 10 commits into from
May 21, 2024

Conversation

szabo137
Copy link
Member

@szabo137 szabo137 commented May 16, 2024

With this, we update the process and cross-section interface to adopt the new phase space points. This solved #57.

TODOs

  • update tests for process interface
  • update test implementation for processes
  • update process interface description
  • update tests for cross-section and probability
  • update building of cross sections and probabilities
  • update perturbative compton
  • old interface (incl. versions for vectors of inputs)
  • cleanup

@szabo137 szabo137 changed the title Update process interface interface [WIP] Update process interface interface May 16, 2024
@szabo137 szabo137 marked this pull request as ready for review May 16, 2024 22:20
@szabo137 szabo137 changed the title [WIP] Update process interface interface Update process interface interface May 16, 2024
@szabo137
Copy link
Member Author

szabo137 commented May 16, 2024

@AntonReinhard I think this is ready for review. You are right, deleting lots of code is a lot of fun 😆

BTW: In contrast to #57 the functions _incoming_flux and _total_probability can not be defined on PhaseSpacePoint, because the latter always contain outgoing momenta, which these functions do not depend on. In general, there are cases, where one wants a PhaseSpacePoint but only with the incoming particles: an IncomingPhaseSpacePoint, maybe.

@szabo137 szabo137 added this to the Release-next milestone May 17, 2024
@AntonReinhard
Copy link
Member

In contrast to #57 the functions _incoming_flux and _total_probability can not be defined on PhaseSpacePoint, because the latter always contain outgoing momenta, which these functions do not depend on. In general, there are cases, where one wants a PhaseSpacePoint but only with the incoming particles: an IncomingPhaseSpacePoint, maybe.

That makes sense. I think it would be nice to have an IncomingPhaseSpacePoint, but if that one exists, then an OutgoingPhaseSpacePoint probably should too, and having three times the essentially same struct seems bad for code duplication.

I think there are a few options to consider:

  • Have the current PhaseSpacePoint be made up of an incoming and an outgoing PhaseSpacePoint. Those two should be symmetric, so only one definition is really needed. Problem: There'll likely be more memory footprint and checking required, since a PhaseSpacePoint would need to make sure that the process, model, and ps_def of the incoming and outgoing parts are the same. I think this would also nicely allow to have a product definition of vectors of incoming and outgoing halves.
  • Allow our PhaseSpacePoint to have 0 length in or out particle vectors. We could then add some functions like has_incoming and has_outgoing to make sure the required parts are given where needed. Since the length of the vectors are part of the type info it should be possible to dispatch on this too, though I'm not sure if partial template specializations are a thing in Julia. Otherwise, the function definitions might get long again... Something like these:
const IncomingPSP = PhaseSpacePoint{PROC, MODEL, PSDEF, PSELT, IN_P, 0} where {PROC, MODEL, PSDEF, PSELT, IN_P}
const OutgoingPSP = PhaseSpacePoint{PROC, MODEL, PSDEF, PSELT, 0, OUT_P} where {PROC, MODEL, PSDEF, PSELT, OUT_P}

There might be more ideas, but these two came to mind.

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.

I can definitely see your point with the IncomingPhaseSpacePoint being necessary, there are still way too many functions now that are defined on momenta instead of PSPs now, which propagates through a lot of code. I think the real goal is to have all the public-facing functions defined on (I/O)PSPs.

Together with #49, that should get rid of a lot of boilerplate code again. The solution to that issue is likely to just create constructors for the PSP on Reals/SFMs or any other representation. Then there should be no reason for anything to use flat momenta instead of PSPs. It should also make it simple enough for the user to broadcast functions that it's not really necessary to provide those overloads ourselves.

src/QEDprocesses.jl 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 Outdated Show resolved Hide resolved
src/interfaces/process_interface.jl Outdated Show resolved Hide resolved
src/cross_section/internal.jl Show resolved Hide resolved
@szabo137
Copy link
Member Author

In contrast to #57 the functions _incoming_flux and _total_probability can not be defined on PhaseSpacePoint, because the latter always contain outgoing momenta, which these functions do not depend on. In general, there are cases, where one wants a PhaseSpacePoint but only with the incoming particles: an IncomingPhaseSpacePoint, maybe.

That makes sense. I think it would be nice to have an IncomingPhaseSpacePoint, but if that one exists, then an OutgoingPhaseSpacePoint probably should too, and having three times the essentially same struct seems bad for code duplication.

I think there are a few options to consider:

  • Have the current PhaseSpacePoint be made up of an incoming and an outgoing PhaseSpacePoint. Those two should be symmetric, so only one definition is really needed. Problem: There'll likely be more memory footprint and checking required, since a PhaseSpacePoint would need to make sure that the process, model, and ps_def of the incoming and outgoing parts are the same. I think this would also nicely allow to have a product definition of vectors of incoming and outgoing halves.
  • Allow our PhaseSpacePoint to have 0 length in or out particle vectors. We could then add some functions like has_incoming and has_outgoing to make sure the required parts are given where needed. Since the length of the vectors are part of the type info it should be possible to dispatch on this too, though I'm not sure if partial template specializations are a thing in Julia. Otherwise, the function definitions might get long again... Something like these:
const IncomingPSP = PhaseSpacePoint{PROC, MODEL, PSDEF, PSELT, IN_P, 0} where {PROC, MODEL, PSDEF, PSELT, IN_P}
const OutgoingPSP = PhaseSpacePoint{PROC, MODEL, PSDEF, PSELT, 0, OUT_P} where {PROC, MODEL, PSDEF, PSELT, OUT_P}

There might be more ideas, but these two came to mind.

Understood! I agree that having two distinct types for incoming and outgoing phase spaces adds some duplications and boilerplate code.
Having the special cases in the current implementation, which switch off the in- or out-channel seems reasonable. But we should be careful that we do not end up checking for has_incoming over and over again because this adds lots of indirections. Intuitively, having some type of product type sounds like the best solution, but I currently can't see the solution of the cost-benefit calculation.

@AntonReinhard
Copy link
Member

Having the special cases in the current implementation, which switch off the in- or out-channel seems reasonable. But we should be careful that we do not end up checking for has_incoming over and over again because this adds lots of indirections.

Since we have all the information in the type info now, it should be enough to dispatch on IncomingPSP and have any necessary checks done at compile time. I think I like the idea of the aliases.

@szabo137 szabo137 merged commit bc5f82c into QEDjl-project:dev May 21, 2024
4 checks passed
szabo137 added a commit that referenced this pull request May 22, 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

---------

Co-authored-by: Uwe Hernandez Acosta <[email protected]>
Co-authored-by: Uwe Hernandez Acosta <[email protected]>
Co-authored-by: AntonReinhard <[email protected]>
@szabo137 szabo137 mentioned this pull request May 27, 2024
This was referenced Jun 18, 2024
@szabo137 szabo137 added 05 - Enhancement Improvements of existing code X-scattering-process changes related to scattering processes labels Jul 15, 2024
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 X-scattering-process changes related to scattering processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants