Implement Track<> copy-constructor and backing graph-copy infrastructure #205

edcallaghan · 2025-08-21T19:52:35Z

This PR implements a copy-constructor for the Track<> class template. Track objects contain references to stateful members in the form of "hits," "element crossings," and "effects," the types of which subclass KinKal-defined bases, and which may contain references to each other. The construction of an equivalent, but independent and consistent Track<> thus requires reinstantiation of the full relationship graph among its members, the struture of which is defined by the subclass implementations. This is achieved via a recursive, memoized cloning scheme, which requires all track members to provide a reinstantiation (clone()) method, which should make use of a common reallocation record (CloneContext) to ensure that each graph node is only reallocated once; but this simple implementation does not accommodate reference cycles or e.g. explicit self-references.

New classes:

CloneContext: Contains a hash table used to map addresses of to-be-copied Track<>-members to the addresses of their newly-allocated clones. Reallocation is done lazily, so that an attempt to lookup the address of an unreallocated object triggers its reallocation via its clone() implementation, which may itself involve address lookups and clone() calls as necessary.

New functionality:

Track<> class template now contains a CloneContext member, which is used in an explicit copy-constructor to coordinate cloning of member objects. This context is protected so as to be available to subclasses of Track<>, which may need access to the reallocation-map to consistently clone members strictly of the subclass.
Hit<>, ElementXing<>, and Effect<> class templates now each have default clone() methods which throw an exception indicating that explicit cloning must be implemented. No correct default implementation can be provided because arbitrary references in subclasses allow for arbitrary graph structures, for which no infrastructure exists to track. A default exception is preferable to, e.g., a pure-virtual method in that it allows users implementing subclasses to avoid clone() implementations unless they actually need to use them, which provides a lower barrier for adoption and prevents e.g. providing (incorrect) empty implementations just to satisfy the interface requirement, which would then later cause problems if a copy is eventually attempted.
clone() methods are provided for KinKal-level objects, e.g. Domain, DomainWall, and ParameterHit.
clone() methods are provided for the example ScintHit, SimpleWireHit, and StrawXing class templates.
Explicit copy-constructors are provided for "flat" objects which do not contain references, e.g. PiecewiseTrajectory<> and ResidualHit<>.
Various setters/getters added to support classes as necessary.

brownd1978 · 2025-08-25T23:58:18Z

Thanks for the detailed description!
Thinking about the design: is it necessary to make CloneContext payload Track? Why can't it be passed as argument to the copy constructor (and top-level clone function)? It can still be used by subclasses.

edcallaghan · 2025-08-26T15:07:05Z

Defining the CloneContext as a Track member was an interface decision. I don't see a reason that passing in an external context wouldn't also work, it just shifts more responsibility onto "users." As a Track member, someone only needs to know about the CloneContext class if they try to copy an implementation which doesn't have overloaded clones, and thus needs to implement them. If the context is passed from outside, then anyone who wants to make a copy needs to learn figure out what it is, instantiate one, and not misuse it after the copy is completed, even though the implementation is already complete.

So from an interface perspective, I think encapsulating it into Track' is a good thing. Of course the price is that that extra payload gets lugged around wherever the Track` goes. We could clear the map after the copy completes, to keep it light, but because that has to happen after any subclass-level copy functionality completes, it would be at the mercy of subclass implementations.

I guess my preference would be to keep it as an "under-the-hood" detail, but if we want to prioritize the memory footprint then I can try to factor it out.

brownd1978 · 2025-08-26T16:01:15Z

I agree hiding complexity from the end user is a good thing. What if Track owned a (mutable) CloneContext by reference (UniquePtr)? Then it could be empty, with minimal payload for all normal use cases, but could be instantiated at the beginning of the clone process (and cleared at the end).

…

On Tue, Aug 26, 2025 at 8:07 AM Ed Callaghan ***@***.***> wrote: *edcallaghan* left a comment (KFTrack/KinKal#205) <#205 (comment)> Defining the CloneContext as a Track member was an interface decision. I don't see a reason that passing in an external context wouldn't also work, it just shifts more responsibility onto "users." As a Track member, someone only needs to know about the CloneContext class if they try to copy an implementation which doesn't have overloaded clones, and thus needs to implement them. If the context is passed from outside, then anyone who wants to make a copy needs to learn figure out what it is, instantiate one, and not misuse it after the copy is completed, even though the implementation is already complete. So from an interface perspective, I think encapsulating it into Track' is a good thing. Of course the price is that that extra payload gets lugged around wherever the Track` goes. We could clear the map after the copy completes, to keep it light, but because that has to happen after any subclass-level copy functionality completes, it would be at the mercy of subclass implementations. I guess my preference would be to keep it as an "under-the-hood" detail, but if we want to prioritize the memory footprint then I can try to factor it out. — Reply to this email directly, view it on GitHub <#205 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAH572GWMIR5S4PKSIC4Y33PRZ27AVCNFSM6AAAAACEPXIWEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMRUGU3TIOJTGQ> . You are receiving this because you commented.Message ID: ***@***.***>

-- David Brown ***@***.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

edcallaghan · 2025-08-26T17:20:28Z

Latest push stores the context as a std::unique_ptr member. This should save some 10s of bytes per track when no copies are made. If copies are made, the onus is on the subclass of Track<> to clear the context when it's finished cloning subclass-level data, otherwise the map payload will persist.

brownd1978

Since CloneContext is exposed to every class except Track, the value of keeping it as a 'hidden' data member of Track seems marginal. I also worry about the state of the embeded CloneContext being indeterminate. Instead please consider having only clone functions (not copy constructors) all down the line, and require them all to have a CloneContext. The CloneContext lifetime can then be managed externally, which is the only place that makes sense. That seems more consistent with your overall design.

Fit/Effect.hh

General/CloneContext.hh

brownd1978 · 2025-08-28T20:24:09Z

Trajectory/PiecewiseTrajectory.hh

+    auto rv = std::make_shared< PiecewiseTrajectory<KTRAJ> >();
+    for (auto const& ptr : pieces_){
+      auto piece = context.get(ptr);
+      rv->append(*piece);


append performs geometric operations and matches time ranges. Pushing cloned traj segments directly into pieces_ should produce a more literal copy.

This has been adopted. I wasn't sure about the functionality inside append/prepend, but a quick test with a simpler copy scheme seems to work just as well.

brownd1978

A few more comments...

Trajectory/ClosestApproach.hh

…opy_constructor

edcallaghan · 2025-09-03T21:49:11Z

Since CloneContext is exposed to every class except Track, the value of keeping it as a 'hidden' data member of Track seems marginal. I also worry about the state of the embeded CloneContext being indeterminate. Instead please consider having only clone functions (not copy constructors) all down the line, and require them all to have a CloneContext. The CloneContext lifetime can then be managed externally, which is the only place that makes sense. That seems more consistent with your overall design.

I've explored the road of avoiding a copy-constructor, and it doesn't lead somewhere clean. The simplest reason is that constructors are naturally accommodating of subclasses (i.e., a user doesn't actually call Track::Track, at least not in Mu2e --- it is called during initialization of a subclass). It would be tricky to implement the same thing with cascaded clone calls, which isn't meant to stride over class hierarchies. I have factored out the CloneContext, though, to be passed as an argument and thus be managed externally.

brownd1978

Thanks for making the context external, I believe that will improve long-term flexibility.
Please review the 'set*' functions, my experience has been these can be replaced by appropriate copy and operator = calls, resulting in cleaner (if slightly less efficient) code. If they are truely necessary, see if they can't be made private, as they always open the possibility of a user creating a self-inconsistent object.
I know this PR has been merged into 206, please make changes there.

brownd1978 · 2025-09-09T18:07:43Z

Detector/ParameterHit.hh

      unsigned nDOF() const override { return ncons_; }
      Parameters const& constraintParameters() const { return params_; }
      PMASK const& constraintMask() const { return pmask_; }
+      Weights pweight() const { return pweight_; };


Is there a reason you duplicated constraintMask function?
These new accessors have cryptic names, can they be improved?

I added these methods very mechanically and without much thinking --- the duplicate has been removed and the names of the others are now more descriptive.

brownd1978 · 2025-09-09T18:09:30Z

Detector/ResidualHit.hh


  template <class KTRAJ> class ResidualHit : public Hit<KTRAJ> {
    public:
+      // clone op for reinstantiation


Is this comment still relevant?

Commentary has been revised to be relevant and accurate.

brownd1978 · 2025-09-09T18:10:49Z

Examples/StrawXing.hh

      StrawXing(PCA const& pca, StrawMaterial const& smat);
      virtual ~StrawXing() {}
+      // clone op for reinstantiation
+      // ejc TODO does typeof(tpca_) == ClosestApproach<> need a deeper clone?


Is this comment still relevant?

Commentary has been revised to be relevant and accurate.

brownd1978 · 2025-09-09T18:13:27Z

Fit/DomainWall.hh

+      auto const& prevWeight() const { return prevwt_; }
+      auto const& nextWeight() const { return nextwt_; }
+      auto const& fwdCovarianceRotation() const { return dpdpdb_; }
+      void setPrevPtr(DOMAINPTR const& ptr){ prev_ = ptr; }


Can these set functions be replaced by simply constructing a new object from 2 Ptrs?

A non-public constructor could maybe be worked out, but I've now reduced the access of these methods to private.

edcallaghan · 2025-09-09T21:53:17Z

With one exception, newly-added public setter methods have been changed to private access. The exception is of course ClosestApproach, which will require a larger overhaul to obviate the need for a public modifier.

brownd1978

Thanks for making the updates, this looks good now.

edcallaghan added 4 commits August 6, 2025 14:24

implement Track copy-constructor and supporting infrastructure

41309fe

exchange pure-virtual base clones for runtime exception

5680f31

explicit-default copy/assignment operators

cc48094

explicit override tags

3c08daa

store Track CloneContext member as pointer for smaller base footprint

49342d3

edcallaghan mentioned this pull request Aug 26, 2025

Implement KKTrack<> copy-constructor Mu2e/Offline#1578

Merged

brownd1978 reviewed Aug 28, 2025

View reviewed changes

Trajectory/ClosestApproach.hh Show resolved Hide resolved

edcallaghan added 4 commits September 2, 2025 15:47

flat serial copy of trajectory pieces

a4ead80

Merge branch 'main' of https://github.com/KFTrack/KinKal into track_c…

4860fa6

…opy_constructor

rm bfield usages in DomainWall

4115913

Track copy-constructor accepts external CloneContext

33430f4

brownd1978 approved these changes Sep 9, 2025

View reviewed changes

edcallaghan added 3 commits September 9, 2025 12:36

descriptive accessor names + commentary cleanup

28115fb

restrict (most) new setters to private access

07025b0

proper ClosestApproach reinstantation in examples

05e472a

brownd1978 approved these changes Sep 10, 2025

View reviewed changes

brownd1978 merged commit 3c80e4a into KFTrack:main Sep 10, 2025
4 checks passed

Implement Track<> copy-constructor and backing graph-copy infrastructure #205

Implement Track<> copy-constructor and backing graph-copy infrastructure #205

Uh oh!

Conversation

edcallaghan commented Aug 21, 2025

Uh oh!

brownd1978 commented Aug 25, 2025

Uh oh!

edcallaghan commented Aug 26, 2025

Uh oh!

brownd1978 commented Aug 26, 2025 via email

Uh oh!

edcallaghan commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brownd1978 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brownd1978 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edcallaghan commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brownd1978 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edcallaghan commented Sep 9, 2025

Uh oh!

brownd1978 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

edcallaghan commented Aug 26, 2025 •

edited

Loading

brownd1978 left a comment •

edited

Loading

edcallaghan commented Sep 3, 2025 •

edited

Loading