Inconsitent definitions of redundant accesors (`x`<->`px`)

[m-fila]

The current coordinate systems XYZT, PxPyPzE, PtEtaPhiM are very inconsistent what and how they define. For instance in PxPyPzE.jl there are defined accessors for any coordinate system

LorentzVectorBase.jl/src/coordinate_systems/PxPyPzE.jl

Line 52 in b9b9d8f

x(cs, p) = px(p)

The correspondence between x, y, z,t and px,py,pz,E is mentioned only in their docstring. It's not reflected in the "interface" code (only added ad-hoc in PxPyPzE.jl).
I'm not sure if this can be automatically enforced (or even desired), but I think the provided coordinate systems should follow the design described in the docs. that is defining all accessors from except accessors from coordinate_names. In particular XYZT with coordinate_names=(:x, :y, :z, :t) shouldn't define x, y,z,t and leave them to vector types

[szabo137]

Many thanks for reporting your concerns.

I partially get your point and try to explain the idea: the coordinate system PxPyPzE is a bit special, because it should act as a kind of alias to XYZT. The construction you see in PxPyPzE.jl line 52ff is there to avoid ambiguities, because the user-facing interface for the coordinate system PxPyPzE should be implemented analogously to every other coordinate system, despite internally it calls the XYZT.jl accessors instead. Therefore, nothing seems inconsistent when it comes to the user-facing interface.

Having that, could you please explain in more detail what you mean by inconsistent regarding the other coordinate systems, especially PtEtPhiM? A MWE, or an additional but failing unit test showing the inconsistency/ambiguity would be highly appreciated 🙆‍♂️

[m-fila]

Hi Uwe, thanks for you time. I think in the end users get to work with correct interface. all the test are passing etc - I'm very pedantically concerned about the organization of current implementation and whether it could be more explicit. In particular, I believe the coordinate systems should fulfill the requirements for "coordinate system" as described in the docs, and overall be a good example how "coordinate systems" are supposed to be implemented.

I understand that PxPyPzEis supposed to act functionally as alias to XYZT with the difference that it uses different coordinate names. To my understanding it means that:

• px(::XYZT, mom), py(::XYZT, mom), pz(::XYZT, mom). E(::XYZT, mom) should be defined
• x(::XYZT, mom), y(::XYZT, mom), z(::XYZT, mom). t(::XYZT, mom) shouldn't be defined
• px(::PxPyPzE, mom), py(::PxPyPzE, mom), pz(::PxPyPzE, mom). E(::PxPyPzE, mom) shouldn't be defined
• x(::PxPyPzE, mom), y(::PxPyPzE mom), z(::PxPyPzE, mom). t(::PxPyPzE, mom) should be defined
• remaining functions are fully implemented for ::XYZT , while ::PxPyPzE functions delegate to them

Currently the functions that shouldn't be defined are defined. Also the interface specification doesn't explicitly describe relationship between x and px. etc. yet x(cs, p) = px(p) is defined which in principle can affect any coordinate system - if this is official behavior then it should be documented and moved from PxPyPz.jl file to interface.jl or somewhere else - I'd like the coordinate system definition files to be self contained - I find it uninitiated that some methods for XYZT are defined in PxPyPz.jl instead of XYZT.jl. same with PtEtPhiM relaying on methods defined in PxPyPz.jl

In #38 O tried to make the coordinate systems definitions scoped to just a single file, be more explicit and don't define anything not needed

[szabo137]

Hi, I understand your point. I try to explain the current construction. For a coodinate system CS with coordinate names (:comp1,:comp2,:comp2,:comp4) a custom momentum type Mom must implement coordinate_system(::Mom) = CS() and

comp1(mom::Mom) = ... # not comp1(::CS, mom::Mom)
comp2(mom::Mom) = ... # not comp2(::CS, mom::Mom)
comp3(mom::Mom) = ... # not comp3(::CS, mom::Mom)
comp4(mom::Mom) = ... # not comp4(::CS, mom::Mom)

Therefore, for PxPyPzE one needs to implement px(mom), py(mom), pz(mom), E(mom) and not px(::PxPyPzE, mom), .... Every other accessor implementation (including x(mom), y(mom), z(mom), t(mom)) is then implemented in the PxPyPzE.jl file.

In this context, the only line(s) that might seem off are the ones you mentioned: x(cs, p) = px(p), etc. These are generic fallbacks to avoid code duplication in the implementations of other coordinate systems. For instance, in PtEtaPhiM.jl you find

px(::PtEtaPhiM, v) = pt(v) * cos(phi(v))
py(::PtEtaPhiM, v) = pt(v) * sin(phi(v))
pz(::PtEtaPhiM, v) = pt(v) * sinh(eta(v))

function E(::PtEtaPhiM, mom)
  return sqrt(px(mom)^2 + py(mom)^2 + pz(mom)^2 + invariant_mass2(mom))
end

Without x(cs, p) = px(p), etc, one must duplicate this and add

x(::PtEtaPhiM, v) = pt(v) * cos(phi(v))
y(::PtEtaPhiM, v) = pt(v) * sin(phi(v))
z(::PtEtaPhiM, v) = pt(v) * sinh(eta(v))

function t(::PtEtaPhiM, mom)
  return sqrt(px(mom)^2 + py(mom)^2 + pz(mom)^2 + invariant_mass2(mom))
end

Finally, with the whole construction

x(cs, p) = px(p)
x(::XYZT, p) = x(p)
px(::PxPyPzE, p) = px(p)
px(::XYZT, p) = x(p)

One can decide if one wants to implement x(mom) or px(mom) for the coordinate system XYZT or PxPyPzE. Every combination should work.

However, I agree with you that this might be a bit misplaced and should definitely be documented better.