Skip to content
Snippets Groups Projects

Velo format

Open Paul Seyfert requested to merge VeloFormat into master

asdfghjkl;

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
67 67 // Standard constructor, initializes variables
68 68 //=============================================================================
69 69 PrPixelTracking::PrPixelTracking( const std::string& name, ISvcLocator* pSvcLocator )
70 : Transformer( name, pSvcLocator,
71 {KeyValue{"ClusterLocation", LHCb::VPClusterLocation::Light},
72 KeyValue{"ClusterOffsets", LHCb::VPClusterLocation::Offsets}},
73 KeyValue{"OutputTracksName", LHCb::Event::v2::TrackLocation::Velo} ) {
70 : MultiTransformer( name, pSvcLocator,
71 {KeyValue{"ClusterLocation", LHCb::VPClusterLocation::Light},
72 KeyValue{"ClusterOffsets", LHCb::VPClusterLocation::Offsets}},
73 {KeyValue{"BackwardStates", LHCb::Hlt1Event::VeloTESLocation::BackwardVertexingStates},
74 KeyValue{"BackwardSel", LHCb::Hlt1Event::VeloTESLocation::BackwardVertexingStateSel},
75 KeyValue{"ForwardHits", LHCb::Hlt1Event::VeloTESLocation::ForwardHits},
  • Is there no "BackwardHits" just because these aren't needed by VeloUT etc. to make old-style Track objects/do MC matching?

  • indeed, the idea was that backward tracks don't end up in the fit / physics. but i didn't check if MC matching is done with backward tracks.

  • Please register or sign in to reply
  • Olli Lupton
    Olli Lupton @olupton started a thread on commit 67346747
  • 104 109 //=============================================================================
    105 110 // Main execution
    106 111 //=============================================================================
    107 std::vector<PrPixelTracking::Track> PrPixelTracking::
    108 operator()( const EventContext& ctx, const std::vector<LHCb::VPLightCluster>& clusters,
    112 PrPixelTrackingOutput PrPixelTracking::
    113 operator()( const EventContext& ctx, const std::vector<LHCb::VPLightCluster>& clusters,
    109 114 const std::array<unsigned, VeloInfo::Numbers::NOffsets>& offsets ) const {
    115 Zipping::ZipContainer<SOA::Container<std::vector, LHCb::Hlt1Event::v1::AtVertexState>> outputVertexStatesBackward;
    116 auto zipid = outputVertexStatesBackward.zipIdentifier();
  • Olli Lupton
    Olli Lupton @olupton started a thread on commit 67346747
  • 607 PrPixelTrackingInternalOutputStruct& outputTracks ) const {
    608 // Decide if this is a forward or backward track.
    609 // Calculate z where the track passes closest to the beam.
    610 const float zBeam = track.zBeam();
    611 // Define backward as z closest to beam downstream of hits.
    612 const bool backward = zBeam > track.hitsZ().front();
    613
    614 if ( !backward ) {
    596 615 // In Forward approach ids already reversed-sorted if lhcbID is increasing for increasing moduleID (direct order for
    597 616 // backward tracks)
    598 std::vector<LHCb::LHCbID> ids;
    599 std::vector<LHCb::TrackHit> trackHits;
    600 ids.reserve( hitbuffer.size() );
    601 trackHits.reserve( hitbuffer.size() );
    617 outputTracks.fwdHits->emplace_back();
    618 auto hits = outputTracks.fwdHits->back();
    • I got a bit lost in the types. What type is hits here? I expected to see auto& not auto given what comes next, but I guess the semantics are a bit different.

    • (worth double checking once it's running)

      It should be a proxy object (i.e. skin) of VeloTrackHits, these don't need to be passed by reference because how proxies work. not ideal for reading indeed.

    • Please register or sign in to reply
  • Olli Lupton
    Olli Lupton @olupton started a thread on commit 67346747
  • 600 ids.reserve( hitbuffer.size() );
    601 trackHits.reserve( hitbuffer.size() );
    617 outputTracks.fwdHits->emplace_back();
    618 auto hits = outputTracks.fwdHits->back();
    619 hits.reserve( hitbuffer.size() );
    620 // ids trackHits
    602 621
    603 622 if ( configuration == SearchDirection::Forward ) {
    604 623 // tracks are created by large z to small z, lhcbID ordered with increasing z, reverse iteration on hit buffer.
    605 624 for ( unsigned i = hitbuffer.size(); i-- != 0; ) {
    606 ids.push_back( clusters[hitbuffer[i]].channelID() );
    607 trackHits.emplace_back( MakeFitterHit( vpInfo, clusters[hitbuffer[i]] ) );
    625 hits.push_back( clusters[hitbuffer[i]].channelID(), MakeFitterHit( vpInfo, clusters[hitbuffer[i]] ) );
    608 626 }
    609 627 }
    610 628 if ( configuration == SearchDirection::Backward ) {
    • We're inside if ( !backward ) here. Are there two different meanings of forward/backward, or will this inner if just never be true? (just for my education).

    • tbh I didn't question the SearchDirection logic and how search direction and track direction interact. (iirc this relation also changes in Arthur's implementation)

    • Please register or sign in to reply
  • Olli Lupton
    Olli Lupton @olupton started a thread on commit 67346747
  • 611 629 // tracks are created by small to larger z, lhcbID ordered with decreasing z
    612 630 for ( size_t hit : hitbuffer ) {
    613 ids.push_back( clusters[hit].channelID() );
    614 trackHits.emplace_back( MakeFitterHit( vpInfo, clusters[hit] ) );
    631 hits.push_back( clusters[hit].channelID(), MakeFitterHit( vpInfo, clusters[hit] ) );
    615 632 }
    616 633 }
    617 634 if ( configuration == SearchDirection::Default ) {
    618 635 for ( size_t hit : hitbuffer ) {
    619 ids.push_back( clusters[hit].channelID() );
    620 trackHits.emplace_back( MakeFitterHit( vpInfo, clusters[hit] ) );
    636 hits.push_back( clusters[hit].channelID(), MakeFitterHit( vpInfo, clusters[hit] ) );
    621 637 }
    622 std::sort( ids.begin(), ids.end() );
    623 std::sort( trackHits.begin(), trackHits.end(),
    638 std::sort( hits.veloHitBlock().m_VeloLHCbIDContainer.begin(), hits.veloHitBlock().m_VeloLHCbIDContainer.end() );
    • I suppose in general this means that you lose the LHCbID-TrackHit relationship. But fine, not new to this commit.

    • not sure I understand your suspicion. the idea is that a track's LHCbIDs get sorted, not all of VeloTracking's IDs, so the track container and the major-index of the LHCbIDs remain aligned

    • My understanding is that 1 track has 1 VeloHitBlock which contains vector<LHCbID> ids and vector<TrackHit> hits -- with the exception of SearchDirection::Default then ids[i] is the LHCbID corresponding to TrackHit hits[i], but the std::sort will break that.

    • i take it the variable names are not good at all.

      hits is just the hits of one individual track.

    • My fault for using bad names in my examples. I think my point still stands: with SearchDirection::Default then for one individual track you have small_vector<TrackHit, ...> hits_for_this_one_track and small_vector<LHCbID, ...> ids_for_this_one_track but hits_for_this_one_track[i] does not correspond to the same "hit" as ids_for_this_one_track[i] for any given i.

      Edited by Olli Lupton
    • AH! now the penny dropped, I understood you wrong.

      Yes, these two are unaligned.

    • OK :thumbsup:

      Below threshold for now, especially if Arthur's implementation is different anyway.

    • Please register or sign in to reply
  • Olli Lupton
    Olli Lupton @olupton started a thread on commit 67346747
  • 120 PrPixelTrackingInternalOutputStruct buffer{&outputVertexStatesBackward, &outputHits, &outputVertexStates,
    121 &outputTrackingStates};
    110 122
    111 123 const auto& vpInfo = m_vpInfo.get( getConditionContext( ctx ) );
    112 124
    113 125 //---- Get for each cluster the phi value, clusters have been already sorted in VPClus algorithm
    114 126 const std::vector<float> phi_hits = getPhi( clusters, offsets );
    115 auto outputTracks = searchByPair( vpInfo, clusters, offsets, phi_hits );
    116 m_tracksCounter += outputTracks.size();
    127 searchByPair( vpInfo, clusters, offsets, phi_hits, buffer );
    128 m_tracksCounter += ( outputVertexStatesBackward.size() + outputVertexStates.size() );
    117 129 m_clustersCounter += clusters.size();
    118 return outputTracks;
    130
    131 Zipping::ExportedSelection<> backwardSel( outputVertexStatesBackward, Zipping::details::alwaysTrue{} );
    132 Zipping::ExportedSelection<> trivialSel( outputHits, Zipping::details::alwaysTrue{} );
    • We should probably get in the habit of adding asserts in places like this to check that outputVertexStates, outputTrackingStates and outputHits all ended up the same length. I'm not sure that this gets checked otherwise?

    • good idea. maybe we can go that far and just zip the outputs and create the exported selection based on the zip. (SOAContainer checks lengths on zipping, and we get the zipIdentifier check).

      though SOAContainer checks the lengths in all builds while the assert would disappear in optimised builds.

    • What would it look like in this case? Would the return type be something like tuple<backwardstates, backwardstates_selection, zip<forwardstates, forwardtrackingstates, forwardhits>, forward_zip_selection>?

    • unfortunately not. I was thinking

      ... operator() ... {
        ...
      
        auto forward = semantic_zip<...>(forwardstates, forwardtrackingstates, forwardhits);
        Zipping::ExportedSelection<> forward_selection{forward, Zipping::details::alwaysTrue{} );
      
        return std::tuple<backwardstates, backwardstates_selection, forwardstates, forwardtrackingstates, forwardhits, forward_selection>;
      }

      one can't write non-owning SOA::Views to TES (which the zips are) without also writing the owning SOA::Containers to TES (and if so, one has to write first the owning container, then create the view, and then write the view).

      One alternative that's stuck in my head is to create a big SOA::Container with all columns to only one TES location and rely on no-overhead when reading. (except that breaks the "move exchange format to Rec" idea a bit).

    • OK, and in that example then the forward_selection constructor does some length-checking, but forward_selection doesn't have any memory of forward or the underlying containers themselves, so it doesn't matter that they get moved onto the TES? The only net change is that we get length-checking "for free"?

      Can we have an algorithm (we can call it ProtoParticleMaker) that takes a bunch of owning SOA::Containers (think "everything that we put in a v1::Track") and returns a non-owning SOA::View (+ a selection) containing all that? I'm not sure if that's what you mean? (it makes sense if SOA::Container was meant to be SOA::View in your last paragraph, as-written it sounds like you're suggesting a lot of data-copying so I am confused)

    • Please register or sign in to reply
  • Paul Seyfert
    Paul Seyfert @pseyfert started a thread on commit 67346747
  • 51
    52 operator LHCb::State const&() const { return this->endOfVeloState(); }
    53 operator LHCb::State&() { return this->endOfVeloState(); }
    54 };
    55
    56 SOASKIN( VeloUTInput, EndOfVeloStateField, VeloHitBlockField ) {
    57 SOASKIN_INHERIT_DEFAULT_METHODS( VeloUTInput );
    58
    59 operator LHCb::State const&() const { return this->endOfVeloState(); }
    60 std::size_t nLHCbIDs() const { return this->veloHitBlock().size(); }
    61 // operator LHCb::State&() { return this->endOfVeloState(); }
    62
    63 /**
    64 * @brief Number of Velo hits on the track
    65 */
    66 auto size() const { return this->veloHitBlock().m_VeloHitContainer.size(); }
  • Paul Seyfert
    Paul Seyfert @pseyfert started a thread on commit 67346747
  • 65 69 inline std::ostream& operator<<( std::ostream& os, const ConfAlgo& s ) { return toStream( s, os ); }
    66 70 } // namespace VPConf
    67 71
    72 using PrPixelTrackingOutput =
    73 std::tuple<Zipping::ZipContainer<SOA::Container<std::vector, LHCb::Hlt1Event::v1::AtVertexState>>,
    74 Zipping::ExportedSelection<>,
    • offline discussion: misunderstanding if we're happy to drop this part of the output very soon (before opening the real MR) or if we don't add it in the first place.

    • Please register or sign in to reply
  • Paul Seyfert added 1 commit
  • Paul Seyfert added 1 commit

    added 1 commit

    Compare with previous version

  • Paul Seyfert added 1 commit

    added 1 commit

    Compare with previous version

  • Please register or sign in to reply
    Loading