WIP: Make PackTrack functional

Making PackTrack functional so that tracks can be packed and persisted in Moore.

Note that

• Removed checking functionality EnableCheck
• Removed delete input functionality DeleteInput
• Removed AlwaysCreateOutput option

added all-slots label

@nskidmor I would suggest to avoid all-slots so that we don't break them by accident. You can trigger a ci-test instead

removed all-slots label

changed the description

added 1 commit

• b905620d - Remove AlwaysCreateOutput

Compare with previous version

/ci-test --merge

added ci-test-triggered label

• [2020-09-12 10:42] Validation started with lhcb-master-mr#1300
• [2020-09-14 12:45] Validation started with lhcb-master-mr#1308

why not go one step further and make this a Gaudi::Functional::Transformer?

and while you're at it, why not just put the content of the header file verbatim in the .cpp one?

marked as a Work In Progress

added 1 commit

• e17eabf3 - Making track packer functional

Compare with previous version

changed title from WIP: Give DataHandles to PackTrack to WIP: Make PackTrack functional

changed the description

@graven Is this what you had in mind? It builds and runs just as the datahandle version did. For my knowledge is the namespace name chosen

namespace LHCb::Converters::PackedTrack

deliberately to match

#include "Event/PackedTrack.h"

Kind of yes ;-)

Basically, there are already several algorithms that 'convert' data from one representation to another (eg. Track V2 to Track v2, or 'Pr' tracks to Track V1 or V2, see eg. code here) And the convention kind of is to enclose them in LHCb::Converters (because they convert things), and then indicate the 'target' type, which is the most relevant for any downstream code, and then indicate some way the 'from' type...

and yes, this is what I just as I had in mind -- only difference are some empty lines ;-)

I find this confusing. We used converters for things we hopefully get rid of at some point. Why not put them into a "Packers" namespace?

/ci-test --merge

unmarked as a Work In Progress

mentioned in issue Moore#204 (closed)

Getting errors in Brunel

AttributeError: Configurable 'PackTrack' does not have property 'EnableCheck'.

That's not surprising, as those properties were removed -- so any bit of code trying to set them should be adapted. I suspect that:

diff --git a/GaudiConf/python/GaudiConf/DstConf.py b/GaudiConf/python/GaudiConf/DstConf.py
index 5462aa65b..7ed833c2e 100644
--- a/GaudiConf/python/GaudiConf/DstConf.py
+++ b/GaudiConf/python/GaudiConf/DstConf.py
@@ -313,12 +313,7 @@ class DstConf(LHCbConfigurableUser):
 
         # Pack tracks if they were not read from the input file...
         if "Tracking" not in self.getProp("EnableUnpack"):
-            packDST.Members += [
-                PackTrack(
-                    name="PackTracks",
-                    AlwaysCreateOutput=alwaysCreate,
-                    EnableCheck=doChecks)
-            ]
+            packDST.Members += [ PackTrack( name="PackTracks") ]
             # for Run2, add packed Velo tracks
             if ((self.getProp("DataType") in self.Run2DataTypes)
                     and (self.getProp("PVPersistence") is "RecVertex")):
@@ -394,10 +389,9 @@ class DstConf(LHCbConfigurableUser):
             packDST.Members += [
                 PackTrack(
                     name="PackMuonTracks",
-                    AlwaysCreateOutput=alwaysCreate,
                     InputName="/Event/Rec/Track/Muon",
                     OutputName="/Event/pRec/Track/Muon",
-                    EnableCheck=doChecks)
+                )
             ]
 
         # In MDF case, add a sub sequence for the MDF writing

should do the trick...

If that works, does that mean that we have no tests which test the packing?

Indeed. But note that that property used to drive the following code:

  // Packing checks
  if ( UNLIKELY( m_enableCheck ) ) {
    // make new unpacked output data object
    LHCb::Tracks* unpacked = new LHCb::Tracks();
    put( unpacked, m_inputName + "_PackingCheck" );

    // unpack
    packer.unpack( *out, *unpacked );

    // run checks
    packer.check( *tracks, *unpacked ).ignore();

    // clean up after checks
    StatusCode sc = evtSvc()->unregisterObject( unpacked ).andThen( [&] { delete unpacked; } );
    if ( sc.isFailure() ) return sc;
  }

(interesting how it first registers something in the TES, only to remove it afterwards...)

So if we want a replacement for that I would propose to make a separate Gaudi::Functional::Consumer<void(const LHCb::Tracks&)> algorithm (given that these algorithms are only a handful of lines of code):

namespace LHCb::Packers::Check {
  struct TrackPacker : Gaudi::Functional::Consumer<void(const LHCb::Tracks&)> {
    TrackPacker( const std::string& name, ISvcLocator* pSvcLocator )
      : Consumer::Consumer{name, pSvcLocator, KeyValue{"InputName", TrackLocation::Default} }{}

    void operator()(const LHCb::Tracks& in) const override {
     auto packer = LHCb::TrackPacker{this};
     LHCb::PackedTracks packed;
     packer.pack( in, packed );
     LHCb::Tracks unpacked;
     packer.unpack( packed, unpacked );
     packer.check( in, unpacked ).orThrow( "Packing check failed", name() );
    }
  };
  DECLARE_COMPONENT_WITH_ID( TrackPacker, "CheckTrackPacker" )
}

and then add that algorithm where previously EnableCheck would be enabled... that moves an 'if' from run time to configuration time, which is always a good thing IMHO.

Now, one could perhaps wonder whether a properly written, dedicated unit test for just the packer would not be a better / sufficient solution, instead of parasitically random sampling the phase space of tracks 'on the fly' inside of some larger job, but let's keep that discussion for another occasion...

Aside: if the pack(), and unpack() methods would return *this i.e. return a reference to their instance, instead of being void, you could write it as a daisy chain:

  LHCb::Tracks unpacked;
  LHCb::PackedTracks packed;
  LHCb::TrackPacker{this}.pack( in, packed)
                         .unpack(packed, unpacked)
                         .check( in, unpacked).orThrow("Packing check failed", name());

You may question the way in which the check was implemented, but this was to all intents and purposes a unit test of the packing and unpacking that was implemented as close to the code as could possibly be (and which was run by the author on large statistics samples whenever he made a change). We have for good reasons changed the way in which we test our code, but if you remove this, you should definitely implement an equivalent unit test.

Looking a bit in the configuration it turns out that we do run these checks in Brunel's xdst-to-xdst test (on 10 events, but still something). There the Brunel().Histograms="Expert" is set, which in turn means that the packing checks are enabled.

Since all of this persistence machinery is likely to change, I would suggest to keep the checking in the simplest possible way for now. This means adding back the EnableCheck property, and copy-pasting and adapting the call to the check that was removed. You should be able to simplify it on the way and avoid putting a temporary container on the TES by changing it to something like this:

    if ( UNLIKELY( m_enableCheck ) ) {
      LHCb::Tracks unpacked;
      packer.unpack( out, unpacked );
      packer.check( tracks, unpacked ).ignore();
    }

(does put have a side effect that the old code relied on?)

mentioned in merge request Moore!619 (merged)

Hi @nskidmor, this is effectively a new algo. I would replace Callot's name by yours at this point. He left a decade-ish ago, BTW ... And better write a relevant date too.

Though it would be fair to retain an acknowledgement, since the packing concepts, in particular the machinery for determining the packing precision, remain his. Perhaps mention it is adapted from the run2 version by Olivier.

That's a good point/idea.

added modernisation label

marked as a Work In Progress

assigned to @nskidmor

Any progress on this one ?

I'd say we can close it. We've made good progress in Moore without needing the changes here. Do you agree @nskidmor?

Ah, I noticed the pipeline failure and fixed it just now to continue on something clean :D. But you if say this MR is redundant ...

Yes I agree :) If we choose to go in this direction in the future we can relook at this

added 1 commit

• 1295f6b9 - Fixed formatting

Compare with previous version

closed

changed the description

unassigned @nskidmor

mentioned in merge request !3081 (closed)