Skip to content
Snippets Groups Projects

Draft: Create a container for neutral objects

Closed Miguel Ramos Pernas requested to merge mramospe-neutrals into master

Implements #104. Due to the requirements of the (several) calorimeter reconstruction tools, it is preferred to avoid having a SIMD-type based container for neutral objects. We rely on the use of the zip machinery together with the construction of SIMD-type objects on the fly from the information of the underlying objects.

Used on Rec!2639 (closed) and Moore!1200

(related also to Phys!1024 (closed))

:warning: This MR has been superseded by Rec!3247 (merged) Analysis!942 (merged) :warning:

Edited by Miguel Ramos Pernas

Merge request reports

Merge request pipeline #3785975 passed

Merge request pipeline passed for 7ac1e594

Closed by Miguel Ramos PernasMiguel Ramos Pernas 2 years ago (Jan 10, 2023 12:50pm UTC)

Merge details

  • The changes were not merged into .

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • 7a270d70 - Add test for load_by_invoking

    Compare with previous version

  • added 1 commit

    Compare with previous version

    • thanks for this @mramospe For my understanding, what is missing to have a first working version?

    • I am still trying to make the vertex fitter work with neutrals. Even if we are not using the information for the fit, we have to tell the ThOr combiner how to deal with this kind of objects. Once that is done, I will try to create a dummy line selecting

      B^0\to K^{*0}\gamma
      (better to start with photons than neutral pions) and adapt whatever is needed to parse the photons and build CaloHypothesesWithOrigin (I will consider changing the name of the container in the future, since now they do not store an origin).

      Edited by Miguel Ramos Pernas
    • Thanks for the details. Let me point out a couple of things that are quite particular when making combinations involving neutrals (we probably discussed this already, but just to be sure it's written somewhere):

      • for the standard case we have a decay vertex from where >= 2 tracks and a photon originate (eg B -> Kst (K pi) gamma). In this case, we ideally want the vertex fit to use the information from the tracks only but also that the photon momentum is updated by changing its origin to the position of the vertex, and the B mass is computed from this updated photon momentum.
      • there are more complicated cases, where one cannot actually reconstruct the decay vertex, eg Lb -> Lambda gamma, where the Lambda flies before decaying to p pi. In this case, we don't want to run a vertex fit, but just a mass combiner that uses the original photon momentum and assumes the parent originates at the associated PV.

      I'd suggest to build all these features step by step, so we have a basic solution soon and keep adding features in further iterations, but to keep in mind when designing the solution what we will need in the end.

    • Thanks for writing it here :smile:. I definitely remember these cases. The main problem of altering the values of the photons is that we would need to somehow write a new particle (create a copy of the input photon with a corrected value of the direction of flight) and save it somewhere else. It is not so clear to me how to do this right now. At the moment I am stuck with the vertex fitter, which can be relatively-easy adapted to modify the momenta of the parent particle by returning appropriate values of the energy and flight direction (assuming that it comes from the PV at the moment). My main concern now is that it seems that the ThOr combiner is using the ScatterGather proxy behaviour, which is not currently supported by my accessor by function call due to the fact that I haven't found so far a way to extract the contents of an int_v (SIMD type) to an array of integers (which I can use directly in the function call as indices).

    • Indeed, I completely missed that function. That's what I was looking for.

    • @graven @cmarinbe, the current implementation of Moore for ChargedBasics objects seems to use a Proto2ChargedBasics converter, so I was wondering if we should do something similar with neutrals or whether we could create them straight away from LHCb::Event::Calo::v2::Hypotheses. Are we currently creating (or is there a way to create) LHCb::Event::Calo::v2::Hypotheses in the reconstruction sequence of Moore? Is there also a way to convert the information from a LHCb::ProtoParticle and store it in the Hypotheses container? Maybe we can go straight for the first option, but I we might want consider whether we should validate both methods or not.

    • If we can skip making ProtoParticles, then we should ;-) And yes, Moore creates LHCb::Event::Calo::v2::Hypotheses, if you eg. do git grep Hypo | grep -i operator in Rec, then you'll find CaloFutureMergedPi0 and ClassifyPhotonElectronAlg so with those two you get Hypotheses for photons, electrons and merged pi0.

    • OK, thanks a lot for the quick answer :smile:. I will start using straight away LHCb::Event::Calo::v2::Hypotheses objects then.

    • :thumbsup: and perhaps we should open an issue to make ChargedBasic directly from v3::Track (or v1::Track for now) instead of going through ProtoParticle...

      Edited by Gerhard Raven
    • @graven could you point me to the place in Moore where the LHCb::Event::Calo::v2::Hypotheses objects are created? So far what I see in Hlt/RecoConf/python/RecoConf/calorimeter_reconstruction.py seems to be all v1. There are even some v2-to-v1 converters but they are not used.

    • You're looking at the right thing, but your conclusion isn't quite right -- yes, in the end it returns 'everything v1', but to make 'everything v1' it does so by first making 'everything v2' and then converting back. So I don't think your statement that the converters are not used is correct!

      To be specific, lets consider eg. the returned 'photons' which is defined as:

      "photons": old_photons,

      but then:

      old_photons = HypoConverter(
              InputHypos=photons, InputClusters=old_ecalClusters).OutputHypos

      where 'InputHypos=photons" means this is using:

      PhElOutput = make_photons_and_electrons(best_tracks, ecalClusters, pvs)
          photons = PhElOutput["photons"]

      and then going to make_photons_and_electrons, and there you'll see

      photons = photonElectronAlg.OutputPhotons

      with

      photonElectronAlg = ClassifyPhotonElectronAlg( ... )

      where ClassifyPhotonElectronAlg is this bit of code which here creates the v2 Hypotheses for both electrons and photons (without ever mentioning v1).

      So the fact that make_calo returns a dictionary with v1 objects does not mean that the v2 ones are not made -- it is just for backward compatibility that it (still) returns v1 hypos, but they are all made by converting v2 Hypotheses -- which is also what you see in the flame graphs of Moore, as these converters actually take some time. So it would be good to update the 'consumer' code which still uses v1 hypos (which is AFAIK both the ProtoParticle code and the persistency) to directly use the v2 hypos instead.... (and the persistency still uses v1 because the protoparticles still use it, and therefor that is what it still needs to support)

      Edited by Gerhard Raven
    • Sorry @graven, I didn't realize that you are using aliases to refer to those converters :face_palm_tone1:. Thanks a lot for all the clarification. Then, despite the LHCb::Event::Calo::v2::Hypotheses container is general (can store photons,

      \pi^0
      , ...) these functions seem to create electrons and photons. Is the photon container meant to store any kind of neutral particle, or do we plan to split it depending on the particle ID?

    • By "any kind" of neutral particle what I actually mean is whether all the elements of the containers will have the same particle ID (I see that we are also processing

      \pi^0
      mesons with a different algorithm).

    • In the code the 'hypothesis' enum (eg. the 'thing' flagging whether an entry is photon/electron/merged pi0/photon from merged pi0/ pi0resolved/... ) is stored at the level of the individual elements of the container, so code-wise it is possible to 'mix and match' those within one container. However, IIRC, we currently just do not do that, and electrons, photons and merged pi0 happen to live in separate containers... but again, there is no technical reason for that, it's just (again, IIRC) how it's always used to be...

    • Please register or sign in to reply
  • added 2 commits

    Compare with previous version

  • Gerhard Raven @graven started a thread on commit 1bc25a51
  • 18 18 class CaloHypothesesWithOrigin final {
    19 19
    20 20 protected:
    21 std::vector<Gaudi::XYZPoint> m_origin;
    21 std::vector<Gaudi::XYZPoint> m_direction;
    • There is a distinction between XYZPoint and XYZVector -- the former denotes a location in space, and the latter a direction (with magnitude). So I would think that something called direction would map to the latter instead... so why is this not std::vector<Gaudi::XYZVector> m_direction?

    • I was just playing around with the implementation, so it is simply a leftover from when I was working with the origin. It might be better to wait till I get to a fully working implementation (i.e. having photons and neutral pions working in HLT2) before starting reviewing this. I decided to create this MR in order to show the preliminary implementation of the zipping and the proposal for gathering values from function calls.

    • Please register or sign in to reply
  • added 62 commits

    • 1bc25a51...d3ae34b0 - 60 commits from branch master
    • f2ef245d - Merge branch 'master' into mramospe-neutrals
    • 37bcb1dc - Use 'private' instead of 'protected' for classes marked as 'final'

    Compare with previous version

  • added 1 commit

    • 7fd0efae - Use a XYZVector for the direction

    Compare with previous version

  • Miguel Ramos Pernas resolved all threads

    resolved all threads

  • added 1 commit

    Compare with previous version

  • added 1 commit

    • 43c3cedc - Force the use of a comma after a call to PROXY_METHODS

    Compare with previous version

  • added 4 commits

    Compare with previous version

  • added 1 commit

    • 06bb1403 - Add assigned masses to NeutralBasics

    Compare with previous version

  • added 1 commit

    • ece4fb75 - Integrate the mass and PID accessors in the CALO objects container. Adapt it...

    Compare with previous version

  • added 1 commit

    • eb8c116e - Implement a 'copy_back' member function and allow to convert mask_v types to std::bitset

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading