search

eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 29 forks source link

Unit multiplies should not be applied in generic algorithms #342

Closed faustus123 closed 1 year ago

faustus123 commented 1 year ago

Currently, there are units being applied within the generic algorithms and in the factory code. For example see the following two lines: https://github.com/eic/EICrecon/blob/492e99f682ff68cc86e23b1b6aae1a6f8b5a29e1/src/detectors/EEMC/CalorimeterHit_factory_EcalEndcapNRecHits.h#L30 https://github.com/eic/EICrecon/blob/492e99f682ff68cc86e23b1b6aae1a6f8b5a29e1/src/algorithms/calorimetry/CalorimeterHitReco.cc#L27

If the user specifies 100*MeV in the factory as the value and it is again multiplied by MeV in the generic algorithm, then the units will be applied twice. Currently, this issue is avoided by the use of run_reco_flags.py which sets values with units such that MeV=1. The multiplication by MeV in the algorithm (which uses units where GeV=1) is then effectively applied only once. This will break when run_reco_flags.py is dropped.

A policy should be adopted on where to apply the units. It probably makes the most sense to have it done in the factory near or at where the default value is set. The generic algorithms can then assume all values are in the correct internal units of the C++ code.

DraTeots commented 1 year ago

That actually opens a can of worms... But very important can: What units system should be used for passing data between factories? There are 3 parts about it:

  1. EDM4eic/EDM4hep - there are units in comments. Does they relate to any units convention? (Geant4, ROOT, DD4Hep, Gaudi, whatever).
  2. Non EDM data passing around - what should be units?
  3. Where those multipliers shold be defined and taken from?