search

Mu2e / Offline

Offline software for the Mu2e experiment
Apache License 2.0
8 stars 79 forks source link

Track hit reco and pattern recognition uses unaligned geometry #77

Closed brownd1978 closed 4 years ago

brownd1978 commented 4 years ago

The code producing ComboHits and performing pattern recognition currently uses the unaligned geometry. Those modules need to be refactored to use the aligned tracker geometry.

ryuwd commented 4 years ago

I'd be happy to work on this - are the relevant modules located in TrkPtRec/, CosmicReco/, and TrkHitReco/ ?

brownd1978 commented 4 years ago

Hi Ryun, It would be great if you could look into this. There are several things to verify. First, can you run a standard reco job and check in the debugger which version of tracker geometry is being used to create the TrkStrawHits? My read of the code is that it's using the nominal geometry. You can run standard reco as:> mu2e -c JobConfig/reco/CeEndpoint-mix.fcl /pnfs/mu2e/tape/phy-sim/dig/mu2e/CeEndpoint-mix/MDC2018d/art/a9/62/dig.mu2e.CeEndpoint-mix.MDC2018d.001002_00000000.art --nevts 10 Then, please check what version of geometry is being used to create the ComboHits in TrkHitReco. Again, I believe that's the nominal geometry.

This will need to be run in offline v7_5_4 or a branch thereof, as the current head of Offline is incompatible with the MDC2018 digis. Let me know if you have any questions.

Dave

On Mon, Jan 6, 2020 at 8:23 AM ryuwd notifications@github.com wrote:

I'd be happy to work on this - are the relevant modules located in TrkPtRec/, CosmicReco/, and TrkHitReco/ ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/issues/77?email_source=notifications&email_token=ABAH5755HUVOZEVFSR5NHDLQ4NLGZA5CNFSM4J5RBNC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIF6I2A#issuecomment-571204712, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH574IRLQIVSCDPVLAOZLQ4NLGZANCNFSM4J5RBNCQ .

-- David Nathan Brown Dave_Brown@lbl.gov Office Phone (510) 486-7261 Fax 495-2957 Lawrence Berkeley National Lab MS 50R5008 (50-6026C) Berkeley, CA 94720

ryuwd commented 4 years ago

Hi Dave,

OK, I've checked out v7_5_4 and had a look at KalFit, KalFinalFit in my IDE. Compiling it now.

In 7_5_4, it appears that any Tracker object retrieved by GeomHandle is nominal/unaligned, as GeometryService caches a Tracker object made by TrackerMaker, not AlignedTrackerMaker. . Also, for AlignedTrackerMaker to work, it relies on GeomHandle returning a nominal geometry. It doesn't look like it is / should be replaced by the Aligned Tracker at any point in the geometry service (??).

In KalFit.cc TrkStrawHits are given Straw objects from _tracker->getStraw, where Mu2eDetector::strawElem(...)->straw() should be used instead (??).

The KalFit object used in KalFinalFit_module.cc is passed a Tracker object from GeomHandle, which returns a nominal Tracker object (i.e. not modified by AlignedTrackerMaker) So KalFit is creating TrkStrawHits using nominal Straws.

If the ComboHitCollection persisted in the KalFitData struct, used to make TrkStrawHits, is produced by TrkHitReco/StrawHitReco_module, then that is using (nominal) Straw objects from GeomHandle Tracker.

After v7_5_4 finishes compiling I will run the reco job, go through with the debugger as suggested and check this is correct. At first glance I agree TrkStrawHits, ComboHits/ComboHitCollection (in StrawHitReco_module.cc) are made using the nominal geometry

Ryun

ryuwd commented 4 years ago

OK, ran the job through the debugger. I made one small change to Tracker object and added a debug flag called "ALIGNED" set to false by default. If a Tracker object is returned by AlignedTrackerMaker, ALIGNED is set to true.

I'm guessing that whether alignment constants are provided or not (e.g. by including misalign_epilog.fcl + db text file), the code should be using Tracker objects passing through AlignedTrackerMaker anyway.

Where TrkStrawHit objects are created, the flag is false, so the geometry used is nominal.

Where ComboHits are made in StrawHitReco_module, the flag is also false, so the geom is nominal here too.

I suppose the next step is to refactor KalFit, KalFinalFit, StrawHitReco to use aligned straws? Are combohits produced elsewhere that need to be considered for standard reco?

Best wishes, Ryun

kutschke commented 4 years ago

Hi Ryun,

On Jan 6, 2020, at 1:44 PM, ryuwd notifications@github.com wrote:

OK, ran the job through the debugger. I made one small change to Tracker object and added a debug flag called "ALIGNED" set to false by default. If a Tracker object is returned by AlignedTrackerMaker, ALIGNED is set to true.

We need to discuss this change more broadly. What use case did you have in mind for downstream code needed to know which one they have?

The idea I have is that the aligned and non-aligned objects are of the same type - so they have the same interface but the values they return are different ( presuming that the alignment contants are non-zero).

I'm guessing that whether alignment constants are provided or not (e.g. by including misalign_epilog.fcl + db text file), the code should be using Tracker objects passing through AlignedTrackerMaker anyway.

Where TrkStrawHit objects are created, the flag is false, so the geometry used is nominal.

Where ComboHits are made in StrawHitReco_module, the flag is also false, so the geom is nominal here too.

I suppose the next step is to refactor KalFit, KalFinalFit, StrawHitReco to use aligned straws? Are combohits produced elsewhere that need to be considered for standard reco?

Best wishes, Ryun

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

ryuwd commented 4 years ago

Hi Rob,

The change was temporary just for my debugging session to confirm whether the Tracker object used in various locations of the code were modified by the AlignedTrackerMaker or not (i.e. is the geometry used nominal or not). I agree that the Tracker type should be able to represent any geometrical configuration of the tracker (which it already does), so no change required.

AlignedTrackerMaker makes a copy of Tracker provided by GeomHandle and modifies the positions, rotations, etc of tracker elements, then returns a pointer to this Tracker object. This is the configuration that should be used in track PatRec and reco (I believe).

Best wishes, Ryun

rlcee commented 4 years ago

I've been replying to the email, but it doesn't seem to be appearing in this comment thread, so I'm re-entering my text here. If anyone know why my email is disappearing, let me know.

It looks like the connection with the alignment is occurring here https://github.com/Mu2e/Offline/blob/master/TrkReco/src/KalFit.cc#L363 https://github.com/Mu2e/Offline/blob/master/TrkReco/src/KalFit.cc#L382 It is likely that there are other points where it should also make contact. I'd say the way I left the code was that the alignment showed a reasonable response in tracks, and not that every aspect of aligned tracking was done and tested. So this is an early prototype, an example, and tracking experts should be looking for problems and places to complete the installation. I was coming at it from the perspective of installing a Proditions table, not commissioning tracking. As I recall, we intentionally did not install it in pattern recognition as we thought we should establish that it was necessary before doing that work. But that decision is up to Dave, as far as I'm concerned.

brownd1978 commented 4 years ago

Hi Ray, these lines just affect the material model used in the Kalman fit, not the hit positions themselves. IE they cause aligned geometry to be used when describing the tracks interactions with the straw walls, but don't affect the hit residuals. My pull request for a few weeks back was supposed to use aligned geometry also for the residuals, but according to your tests it doesn't work. My fix was a hack, as unfortunately the geometry used in hit making comes in through a 'common block'-like structure that was inherited from CalPatRec, which is very non-transparent. I hope to address that design soon. Changing the material model will affect the Kalman fit result to 2nd order but is obviously not all we need. I gave Ryun instructions on how to address the additional parts that need to be updated. cheers, Dave

On Mon, Jan 6, 2020 at 1:46 PM Ray Culbertson notifications@github.com wrote:

I've been replying to the email, but it doesn't seem to be appearing in this comment thread, so I'm re-entering my text here. If anyone know why my email is disappearing, let me know.

It looks like the connection with the alignment is occurring here https://github.com/Mu2e/Offline/blob/master/TrkReco/src/KalFit.cc#L363 https://github.com/Mu2e/Offline/blob/master/TrkReco/src/KalFit.cc#L382 It is likely that there are other points where it should also make contact. I'd say the way I left the code was that the alignment showed a reasonable response in tracks, and not that every aspect of aligned tracking was done and tested. So this is an early prototype, an example, and tracking experts should be looking for problems and places to complete the installation. I was coming at it from the perspective of installing a Proditions table, not commissioning tracking. As I recall, we intentionally did not install it in pattern recognition as we thought we should establish that it was necessary before doing that work. But that decision is up to Dave, as far as I'm concerned.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/issues/77?email_source=notifications&email_token=ABAH575IYXBRBFZ4FPFSUILQ4ORDHA5CNFSM4J5RBNC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIG4TCI#issuecomment-571328905, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH57Y7VK2VUBNZOSIYUT3Q4ORDHANCNFSM4J5RBNCQ .

-- David Nathan Brown Dave_Brown@lbl.gov Office Phone (510) 486-7261 Fax 495-2957 Lawrence Berkeley National Lab MS 50R5008 (50-6026C) Berkeley, CA 94720

rlcee commented 4 years ago

but according to your tests it doesn't work.

This was a mis-communication. My statement was that without your PR, I still saw reasonable response to the alignment - moving 1/2 of the planes 100um causes the chi2 to increase x4. That is the only high-level testing I did, so it is perfectly possible that your PR is the right thing to do. I just don't understand how moving the material would cause that change in the chi2 - maybe another bug? Also, I recall we had agreed to not move the material. Anyway, I don't think I'm adding anything useful at this point.

brownd1978 commented 4 years ago

Hi Ray, we have to move the straw coherently as a unit, material + wire (residuals). It's tricky as those are handled separately, I should have looked at your implementation earlier. I think my PR was doing the correct thing, but I thought you also ran with that and saw 0 change WRT baseline?

At this point Ryun has taken over debugging this. Lets wait to see what he finds.

Dave

On Tue, Jan 7, 2020 at 2:53 PM Ray Culbertson notifications@github.com wrote:

but according to your tests it doesn't work.

This was a mis-communication. My statement was that without your PR, I still saw reasonable response to the alignment - moving 1/2 of the planes 100um causes the chi2 to increase x4. That is the only high-level testing I did, so it is perfectly possible that your PR is the right thing to do. I just don't understand how moving the material would cause that change in the chi2 - maybe another bug? Also, I recall we had agreed to not move the material. Anyway, I don't think I'm adding anything useful at this point.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/issues/77?email_source=notifications&email_token=ABAH57YOXNY7PRBYFZZCDL3Q4UBVBA5CNFSM4J5RBNC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIKSFNA#issuecomment-571810484, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH574D4GOSH266T33TRGLQ4UBVBANCNFSM4J5RBNCQ .

-- David Nathan Brown Dave_Brown@lbl.gov Office Phone (510) 486-7261 Fax 495-2957 Lawrence Berkeley National Lab MS 50R5008 (50-6026C) Berkeley, CA 94720

ryuwd commented 4 years ago

Hi All,

Thank you for the helpful input on this.

Thanks again, Ryun

ryuwd commented 4 years ago

Sorry, I mean doc-24870, not doc-28470.

ryuwd commented 4 years ago

@brownd1978, since the Tracking infrastructure was mostly written to make use of Tracker objects, I think a nice way to fix this is to swap out any use of GeomHandle<Tracker> with ProditionsHandle<Tracker>, where the former only seems to return a nominal geometry Tracker.

This would be nice since then it becomes explicit that alignment constants in Proditions DB tables for the current event are correctly applied to the Tracker object used, and making changes deep in the tracking code isn't required past changing the Tracker instance given to setTracker(...) functions.

I guess it's also important to note that a GeomHandle isn't guaranteed to return a geometry representation that takes into account detector conditions, whereas Proditions does? Is this correct?

Or would it be best to swap out code such that Straw elements are retrieved using detmodel->strawElem(StrawId...)->straw()/Mu2eDetector::strawElem(...) ? as in PR #76 ?

I see three GeomHandle<Tracker> in use in KalFinalFit, KalSeedFit, and StrawHitReco modules (v7_5_4). In master/HEAD, CosmicReco also uses GeomHandle.

Cheers, Ryun

rlcee commented 4 years ago

I guess it's also important to note that a GeomHandle isn't guaranteed to return a geometry representation that takes into account detector conditions, whereas Proditions does? Is this correct? The plan is that GeometryHandle will always return the static, nominal geometry. Proditions will return nominal, test, or run-dependent aligned values depending on it's fcl configuration and database entries.

brownd1978 commented 4 years ago

The use case for GeomHandle is when static parameters are sufficient and Proditions isn't available. An example is initializing StrawResponse, which needs to know the lengths of straws to compute reflection parameters. Since it itself is a proditions object its constructor can't rely on the availability of other proditions.

An argument was also made to keep GeomHandle for comparisons against default geometry in analysis. I personally find that uncompelling, as default geometry can be obtained from proditions by setting null alignment.

Dave

On Mon, Jan 13, 2020 at 7:29 AM Ray Culbertson notifications@github.com wrote:

I guess it's also important to note that a GeomHandle isn't guaranteed to return a geometry representation that takes into account detector conditions, whereas Proditions does? Is this correct? The plan is that GeometryHandle will always return the static, nominal geometry. Proditions will return nominal, test, or run-dependent aligned values depending on it's fcl configuration and database entries.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/issues/77?email_source=notifications&email_token=ABAH572D6MTRJZVO2EVG453Q5SCFRA5CNFSM4J5RBNC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIZEIEI#issuecomment-573719569, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH572WSEIX5DWD6P6MXWLQ5SCFRANCNFSM4J5RBNCQ .

-- David Nathan Brown Dave_Brown@lbl.gov Office Phone (510) 486-7261 Fax 495-2957 Lawrence Berkeley National Lab MS 50R5008 (50-6026C) Berkeley, CA 94720

ryuwd commented 4 years ago

Thanks for clarifying this, that has cleared things up for me. Proditions is the way to go then -

An argument was also made to keep GeomHandle for comparisons against default geometry in analysis. I personally find that uncompelling, as default geometry can be obtained from proditions by setting null alignment.

I agree. I would prefer to avoid writing additional code in e.g. Alignment, to compare nominal vs. misaligned (for example), if I can instead change the alignment of the geometry using Proditions, and compare outputs that way.

Alignment with straight cosmic rays will depend on CosmicReco ( and/or straight line kalman ) using Proditions-aligned geom rather than GeomHandle, so that will need to be checked / refactored before I continue using that track data for Millepede or anything like that.

I've made some changes to some of these modules in my fork to make use of Proditions instead, but I'll wait until I properly test (probably will look at residual distributions + debug again) before submitting a PR. Example here: https://github.com/ryuwd/Offline/commit/596d8f02a804547f3e81f20358f7a26fc213eb65?w=1