impy-project / chromo

Hadronic Interaction Model interface in PYthon
Other
31 stars 7 forks source link

Reintroduce the initialisation of EventKinematics derived classes with all expressions of energy #68

Closed afedynitch closed 2 years ago

afedynitch commented 2 years ago

The functionality has been removed to initialize derived objects of EventKinematics using ekin, plab. One of the purposes of EventKinematics was to properly convert between these types. Please reintroduce all the specifications, such as FixedTarget(ekin=xxx,...) or FixedTarget(ecm=...), or CenterOfMass(elab=...). The title of the class refers to the output frame. This should be made clear in the docs.

Thanks.

HDembinski commented 2 years ago

No, this is going against the original idea. FixedTarget should have only one energy, namely the energy in the fixed target frame. Likewise centerofmass should have ecm only. A class should have one clear purpose not several responsibilities mixed up.

If you want something mixed, frame different from initial state, you can do that by changing the user frame, which you can set in the generator after it was initialized. FixedTarget or CenterOfMass set the energies and the frame, because that's what 99% of the users need. Those which need to initialize in one frame but need to get outputs in another are a special case. They should set the user frame manually after initialization.

afedynitch commented 2 years ago

I think the responsibility of the class is to make sure the output frame is correct. If you have a number like 13 TeV center of mass, and you want to understand what are the secondaries in lab frame (or airshower frame) then you need to use a calculator? I don't think that restricting the definition makes is really needed or goes against the idea. If an experiment defines plab, like NA61 or NA49, how should that be passed?

The purpose of the EventKinematics was to actually reduce user error by providing the input frame in the parameters it is given.

afedynitch commented 2 years ago

@HDembinski I'm migrating MCVD to the new impy. So the case is very common that we have a fixed target experiment, like NA22, and they describe the projectile by plab (here for K+ + p) but the results are given in center of mass frame because nobody is reporting (pseudo-)rapidity in lab frame. So what was done before was EventKinematics(plab=plab, p1pdg=321, p2pdg=2212) but the pseudorapidity or Feynman-x is in center of mass. So it is very common and has nothing to do with dual purposing the class. IMO, if we define the purpose of the class to ensure the correct output frame then there is no ambiguity.

HDembinski commented 2 years ago

I think the responsibility of the class is to make sure the output frame is correct. If you have a number like 13 TeV center of mass, and you want to understand what are the secondaries in lab frame (or airshower frame) then you need to use a calculator?

You can set the user_frame of the generator after initialization, no?

HDembinski commented 2 years ago

@HDembinski I'm migrating MCVD to the new impy. So the case is very common that we have a fixed target experiment, like NA22, and they describe the projectile by plab (here for K+ + p) but the results are given in center of mass frame because nobody is reporting (pseudo-)rapidity in lab frame. So what was done before was EventKinematics(plab=plab, p1pdg=321, p2pdg=2212) but the pseudorapidity or Feynman-x is in center of mass.

Then keep the original EventKinematics as an expert interface, but don't further overload the functionality of CenterOfMass and FixedTarget. The problem with EventKinematics is that it is not clear what the output frame is, because I can set the initial state in any frame.

So it is very common and has nothing to do with dual purposing the class. IMO, if we define the purpose of the class to ensure the correct output frame then there is no ambiguity.

HDembinski commented 2 years ago

By default, the generator should use the frame of the initial state, that is mostly what the user wants. The output frame can be changed separately with the user_frame attribute of the generator.

Your NA22 case seems rather special to me and you are an expert user, not the ordinary user. It is not what the majority of users want.

HDembinski commented 2 years ago

To be more clear, I think it is fine in FixedTarget to also allow setting the momentum instead of the energy, but then we should introduce some tag types.

# total energy type
class Energy(float): ...

# kinetic energy type
class KinEnergy(float): ...

# momentum type
class Momentum(float): ...

# usage
FixedTarget(Momentum(1.2 * GeV), ...)

FixedTarget(Energy(1.2 * GeV), ...)

FixedTarget(KinEnergy(1.2 * GeV), ...)

For CenterOfMass I would not allow anything other than ECM.

HDembinski commented 2 years ago

It is bad design to make function which can accept two conflicting inputs at the same time, like energy AND momentum. There should be only one argument and that argument can accept an energy or a momentum. The function then needs metadata whether this number is an energy or momentum, and that metadata is given by the type.

There are many advantages: the input validation of the function becomes very simple and it is an intuitive API. It is also very explicit and hard to use incorrectly.

HDembinski commented 2 years ago

I recommend to read and contemplate the Zen of Python, it is excellent design advice and should be taken serious despite the joking format. https://peps.python.org/pep-0020/