Track states #16

andresailer · 2017-09-20T16:50:05Z

Implementd LCTrack class to hold multiple track states at calorimeter
Adapted TrackClusterAssociationAlgorithm to check different track states
Adapted ClusterHelper::GetTrackClusterDistance to take a trackState instead of a track so that we can pass the relevant trackState to the function in TrackClusterAssociationAlgorithm, adapt other uses of the function.

For my test sample of 2000 10GeV pions in the relevant direction (34 to 36 degrees polar angle) of the CLIC_o3_v13 detector I find the following number of reconstructed particles

Particle	Before	After
Neutron	185	9
Gamma	90	58
Electron	29	35
Muon	20	22
Pi-	1676	1876

A lot less neutrons, so cluster are better matched now. I have only compared the first entries from the ReconstructedParticle collection, so the reconstruction performance is probably even better than what is shown in the table. Sometimes particles shower before reaching the calorimeter, so perfect results cannot be expected.

PandoraPFA

Suggest we have one more round of discussion, then maybe Cambridge should quickly test the persistency of the additional TrackStates, or, at least, elaborate on the brief testing instructions attached below.

PandoraPFA · 2017-09-20T17:03:16Z

include/LCHelpers/ClusterHelper.h

     *  @param  pCluster address of the cluster
     *  @param  maxSearchLayer the maximum pseudolayer to examine
     *  @param  parallelDistanceCut maximum allowed projection of track-cluster separation along track direction
     *  @param  minTrackClusterCosAngle min cos(angle) between track and cluster initial direction
     *  @param  trackClusterDistance to receive the track cluster distance
     */
-    static pandora::StatusCode GetTrackClusterDistance(const pandora::Track *const pTrack, const pandora::Cluster *const pCluster,
+    static pandora::StatusCode GetTrackClusterDistance(const pandora::TrackState &trackState, const pandora::Cluster *const pCluster,
        const unsigned int maxSearchLayer, const float parallelDistanceCut, const float minTrackClusterCosAngle, float &trackClusterDistance);



Might want to add three specific functions, with first argument being a Track (assuming we're going to use the TrackState at the calorimeter), a TrackState or a vector of TrackStates, with only the single TrackState having the real implementation (other options redirecting to this).

If you prefer, you could template this, with all input arguments being given by address and a single template prototype GetTrackClusterDistance(const T *const pT, ...) and three specialisations providing implementation?

I have implemented this.

PandoraPFA · 2017-09-20T17:03:54Z

include/LCObjects/LCTrack.h

+
+
+
+class LCTrack: public object_creation::Track::Object


Doxygen, whitespace (not going to push too hard on this, though)!

Added @brief doxygen comment, removed some empty lines

PandoraPFA · 2017-09-20T17:15:17Z

include/LCObjects/LCTrack.h

+        for (int i = 0; i < nTrackStates; ++i)
+        {
+            pandora::TrackState trackState(0.0,0.0,0.0,0.0,0.0,0.0);
+            PANDORA_RETURN_RESULT_IF(pandora::STATUS_CODE_SUCCESS, !=, xmlFileReader.ReadVariable("TrackState", trackState));


I'm not sure that the xml parser will like this. It may just read the same (first) TrackState tag multiple times. I think the xml parsing might need to be handled at a lower level here (unfortunately), with the xml node being forced on to its "NextSibling()"

It would actually be quite good to have these functions in place (although it isn't mandatory), because Steve and I foresee using Pandora persistency and the Pandora standalone development environment as a preferred way of maintaining the LC reconstruction e.g. testing that there's no degradation in LC performance when we make LArTPC-motivated changes to the SDK.

Testing is a bit involved if you've not done it before. What you need to do (or Steve might have time to take a look; we haven't talked about this in detail yet) is implement a LC-specific version of the SDK-provided EventWriting and EventReading algorithms (could inherit most functionality). You'd then need to register your factory objects with the EventWriter/EventReader instance.

To test, you'd then run a LC job, with an EventWriting algorithm instance running as e.g. the first (might as well, for clarity) algorithm. You can then reproduce the same reconstruction input, then output, using the LCReco entry point application (see LCReco on GitHub), running a standard PandoraSettings file, but with an instance of the EventReading algorithm first up.

We can discuss in more detail, as required. At a quick look, I think the binary version is OK, but the xml version will probably need the change described above.

Then instead, writing out TrackState0,TrackState1,...,TrackStateN ?

Yes, this is a good, pragmatic way forwards.

Can we make the implementation of the LCEventWriting/LCEventReading algorithms another PR in the near-ish future?
From what I see is that one needs, e.g., an LCEventWritingAlgorithm, which instantiates, e.g., an LCXMLFileWriter object, which re-implements the WriteTrack function[*].

Or is there a different way to "register the factory with the EventWriter instance"?

[*] Plus the permutations for Binary and Reader.

We can separate out the two aspects if you prefer (i. getting access to the additional track states, and ii. sorting out the persistency).

There's some examples (a little bit involved) of how we have done this for the LArTPC use case, linked below. The idea is that you just need to register the factories with the pandora:: FileWriter and pandora::FileWriter, then proceed as before.

https://github.com/PandoraPFA/LArContent/blob/master/larpandoracontent/LArPersistency/EventWritingAlgorithm.cc#L113
https://github.com/PandoraPFA/LArContent/blob/master/larpandoracontent/LArPersistency/EventReadingAlgorithm.cc#L140

When writing or reading e.g. a Track, the existing derived binary and xml implementations will pass control to the registered factory to do their "extra" bit of writing/reading before working with the standard object properties described in the SDK.

https://github.com/PandoraPFA/PandoraSDK/blob/master/src/Persistency/BinaryFileWriter.cc#L274
https://github.com/PandoraPFA/PandoraSDK/blob/master/src/Persistency/XmlFileWriter.cc#L283

https://github.com/PandoraPFA/PandoraSDK/blob/master/src/Persistency/BinaryFileReader.cc#L650
https://github.com/PandoraPFA/PandoraSDK/blob/master/src/Persistency/XmlFileReader.cc#L617

Please just let us know how you want to proceed and when you think things are ready to be merged. We'll then take a quick look again and hopefully make the merge (maybe making a clear note to finalise the persistency at a later date, as required).

I have created #17 (assign to me?) to keep a reminder for the LC EventWriting algorithm, which I would try to tackle this week.
Otherwise this code is ready to be merged as far as I am concerned, please have another look.
We can then run larger scale tests.

PandoraPFA · 2017-09-20T17:27:35Z

src/LCTrackClusterAssociation/TrackClusterAssociationAlgorithm.cc

-                    minDistance = trackClusterDistance;
-                    pBestCluster = pCluster;
-                    minEnergyDifference = energyDifference;
+                    if ((trackClusterDistance < minDistance) || ((trackClusterDistance == minDistance) && (energyDifference < minEnergyDifference)))


The reason for this nonsense? Because we once had issues with the ordering of the Clusters coming out of an unordered container. Reproducibility is obviously very important and something we are completely on top of now (but there are code relics dating from when we had low-level exposure to unordered containers and tried to work around the issues).

PandoraPFA · 2017-09-20T17:28:55Z

src/LCTrackClusterAssociation/TrackClusterAssociationAlgorithm.cc

            {
-                for (auto iter = cached_result.first; iter != cached_result.second; ++iter )
+                // save the hash key since we may use it a few times


The diff is quite difficult to parse for this portion of the implementation (mostly provided by our CMS colleagues when they were doing proof-of-principle studies with Pandora), but I had a quick look over your .cc file in full and couldn't see anything awry.

The only change here is indentation, but empty lines are still empty so the diff gets messed up

…o the right place

…ates

…pt Track, TrackState of vector<TrackState>

PandoraPFA · 2017-09-25T16:35:52Z

Merged via commandline.

PandoraPFA · 2017-09-26T14:31:22Z

Any bright ideas about the new Coverity defect associated with this PR (see paste, below)? With the "tainted scalar" (read via xml) issue, it has sometime been sufficient to just add some basic sanity checks of the input value in order to "resolve" the issue.

1 new defect(s) introduced to PandoraPFA/LCContent found with Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)

** CID 1457070: Insecure data handling (TAINTED_SCALAR)
/pandora/package/include/LCObjects/LCTrack.h: 155 in lc_content::LCTrackFactory::Read(object_creation::TrackParameters &, pandora::FileReader &) const()

*** CID 1457070: Insecure data handling (TAINTED_SCALAR)
/pandora/package/include/LCObjects/LCTrack.h: 155 in lc_content::LCTrackFactory::Read(object_creation::TrackParameters &, pandora::FileReader &) const()
149 }
150 }
151 else if (pandora::XML == fileReader.GetFileType())
152 {
153 pandora::XmlFileReader &xmlFileReader(dynamic_castpandora::XmlFileReader&(fileReader));
154 PANDORA_RETURN_RESULT_IF(pandora::STATUS_CODE_SUCCESS, !=, xmlFileReader.ReadVariable("NumberOfTrackStates", nTrackStates));

CID 1457070:  Insecure data handling  (TAINTED_SCALAR)
Using tainted variable "nTrackStates" as a loop boundary.

155 for (int i = 0; i < nTrackStates; ++i)
156 {
157 pandora::TrackState trackState(0.0,0.0,0.0,0.0,0.0,0.0);
158 std::stringstream trackStateName;
159 trackStateName << "TrackState" << i;
160 PANDORA_RETURN_RESULT_IF(pandora::STATUS_CODE_SUCCESS, !=, xmlFileReader.ReadVariable(trackStateName.str(), trackState));

andresailer · 2017-09-26T14:51:31Z

Just call it intentional? Checking that there are less than N (2? 10? 100?) trackStates is not really helpful, because the program will abort if there are less TrackStateX entries than NumberOfTrackStates, right? Can some attacker create a malicious events.pndr file that will take over the victims computer when he reads it in pandora?

Or instead of for use a while loop to read TrackStateX tags until there are no more, then NumberOfTrackStates is no longer needed?

PandoraPFA · 2017-09-26T15:51:04Z

Agreed, just flagging this as intentional is probably best.

andresailer mentioned this pull request Sep 20, 2017

Multiple track states for TrackClusterMatching iLCSoft/DDMarlinPandora#12

Merged

PandoraPFA reviewed Sep 20, 2017

View reviewed changes

andresailer added 5 commits September 21, 2017 11:40

Add LCTrack implementation to store multiple track states

e84be15

TrackClusterAssociationAlgorithm: loop over trackstates for each track

a9aa74d

TrackClusterAssociationAlgorithm: move end of loop over trackstates t…

d84ddc4

…o the right place

LCTrack: implement Read and Write functions

21434c0

TrackClusterAssociationAlgorithm: indent content of loop over trackst…

70785a5

…ates

andresailer force-pushed the trackStates branch 3 times, most recently from 77fa316 to 45d4fa4 Compare September 21, 2017 10:54

ClusterHelper::GetTrackClusterDistance: template the function to acce…

596f0b2

…pt Track, TrackState of vector<TrackState>

andresailer force-pushed the trackStates branch from 45d4fa4 to 596f0b2 Compare September 21, 2017 14:19

andresailer mentioned this pull request Sep 25, 2017

Create LCContent/EventWritingAlgorithm #17

Closed

PandoraPFA closed this Sep 25, 2017

andresailer deleted the trackStates branch September 25, 2017 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track states #16

Track states #16

andresailer commented Sep 20, 2017

PandoraPFA left a comment

PandoraPFA Sep 20, 2017

andresailer Sep 21, 2017

PandoraPFA Sep 20, 2017

andresailer Sep 21, 2017

PandoraPFA Sep 20, 2017

andresailer Sep 21, 2017

PandoraPFA Sep 21, 2017

andresailer Sep 22, 2017

PandoraPFA Sep 22, 2017

andresailer Sep 25, 2017

PandoraPFA Sep 20, 2017

PandoraPFA Sep 20, 2017

andresailer Sep 21, 2017

PandoraPFA commented Sep 25, 2017

PandoraPFA commented Sep 26, 2017

andresailer commented Sep 26, 2017

PandoraPFA commented Sep 26, 2017

Track states #16

Track states #16

Conversation

andresailer commented Sep 20, 2017

PandoraPFA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PandoraPFA commented Sep 25, 2017

PandoraPFA commented Sep 26, 2017

andresailer commented Sep 26, 2017

PandoraPFA commented Sep 26, 2017