Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition of new algorithms to TOFEstimator processor #88

Merged
merged 6 commits into from Feb 26, 2021

Conversation

dudarboh
Copy link
Member

@dudarboh dudarboh commented Feb 8, 2021

BEGINRELEASENOTES

  • TOFEstimators processor: adding new algorithms to calculate the ToF, track length and momentum.
    • New output parameters:
      • TOFClosest --- based on the closest ECAL hit
      • TOFFastest --- based on the fastest ECAL hit
      • TOFCylFit --- extrapolated time from the ECAL hits within a cylinder of a shower core
      • TOFClosestFit --- extrapolated time from the ECAL hits closest to the linear continuation of the track inside the ECAL
      • FlightLength --- the helix track length based on the track parameters from the TrackState at the ECAL
      • MomAtCal --- the momentum based on the track parameters from the TrackState at the ECAL
    • New steering parameters:
      • ProcessorVersion --- changes output between the idr version (before this patch) and the dev (after this patch). Default: idr
      • CylRadius --- the radius within which to select hits for the TOFCylFit method. Default: 5 mm

ENDRELEASENOTES

Important notes

Currently new algorithms work only with simple cases of PFO, meaning: only one cluster and only one track associated with PFO. Other PFOs are ignored.

Flight length calculation are done using the assumption of less than 2*pi change in the phi angle. Meaning that the flight length gives the wrong results for the tracks with more than one helix curls.

Thus, the new algorithms are still optimized to work in the barrel part only. Performance of the TOF estimators in the endcap is currently unknown.

What's new

We added new TOF estimators that show a better performance in terms of less bias in the measurement of pions, kaons and protons masses.

Steering parameter

New steering parameter ProcessorVersion has been added with the possible values: "idr" (default) and "dev".
The previous behavior is set by default for the backwards compatibility.

Output

In the new version the output parameters are modified to:
{"TOFClosest", "TOFFastest", "TOFCylFit", "TOFClosestFit", "FlightLength", "MomAtCalo"}

  • TOF* --- are TimeOfFlight estimated by different algorithms (see bellow).
  • FlightLength --- is a track length of a particle from IP (assuming (0, 0, 0)) to ECAL surface
  • MomAtCalo --- Track momentum at ECAL surface.

Using these parameters the mass of a particle can be calculated.

TOF estimators

The four estimators are available in the new version:

  • TOFClosest --- time of the ECAL hit closest to the extrapolated track position at the ECAL surface with correction for distance between impact point and center of ECAL cell assuming speed of light propagation.
  • TOFFastest --- time of the ECAL hit with the smallest time with correction for distance between impact point and center of ECAL cell assuming speed of light propagation.
  • TOFCylFit --- time estimated with a linear fit of the ECAL hits times in the 5mm cylinder with the main axis parallel to track direction at the ECAL surface vs distance to the impact point of each hit. Then TOF is the bias parameter of a linear fit.
  • TOFClosestFit --- time estimated with a linear fit of the ECAL hits times which are closest to the track line estimated at the ECAL surface (one closest hit per layer) vs distance to the impact point of each hit. Then TOF is the bias parameter of a linear fit.

@dudarboh dudarboh changed the title Development of TOFEstimators processor [WIP] Development of TOFEstimators processor Feb 8, 2021
@rete
Copy link
Contributor

rete commented Feb 8, 2021

@dudarboh Can you:

  • fill the release notes between BEGINRELEASENOTE S and ENDRELEASENOTE S
  • rebase on the ilcsoft master. The CI has been updated

?

@dudarboh
Copy link
Member Author

dudarboh commented Feb 8, 2021

Hi, @rete.

Done. Please check.

@rete
Copy link
Contributor

rete commented Feb 8, 2021

@dudarboh When you open the pull request there is message:


BEGINRELEASENOTE S
- Thank you for writing the text to appear in the release notes. It will show up
  exactly as it appears between the two bold lines
- ...

ENDRELEASENOTE S

TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/include/TOFEstimators.h Outdated Show resolved Hide resolved
TimeOfFlight/include/TOFEstimators.h Outdated Show resolved Hide resolved
TimeOfFlight/include/TOFEstimators.h Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments @dudarboh . I have just one last comment about one change in the xml steering file, that I have missed before. Is that an intentional change or is it left from testing?

TimeOfFlight/scripts/tofestimators.xml Outdated Show resolved Hide resolved
TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
Proper exception throw of invalid ProcVersion

Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed a parenthesis...

TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
@tmadlener tmadlener changed the title [WIP] Development of TOFEstimators processor Addition of new algorithms to TOFEstimator processor Feb 26, 2021
@rete
Copy link
Contributor

rete commented Feb 26, 2021

@dudarboh Last thing to sort out probably is the release notes. Can you just provide a few bullet points for summary?

TimeOfFlight/src/TOFEstimators.cc Outdated Show resolved Hide resolved
@dudarboh
Copy link
Member Author

Release notes are now only brief bullet points.

@dudarboh dudarboh requested a review from rete February 26, 2021 11:47
@rete rete merged commit 1ced811 into iLCSoft:master Feb 26, 2021
@dudarboh dudarboh deleted the tof branch February 26, 2021 12:17
#include "DDRec/Vector3D.h"
#include "EVENT/LCCollection.h"
#include "EVENT/ReconstructedParticle.h"
using EVENT::LCCollection, EVENT::ReconstructedParticle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work!

error: expected ';' before ',' token
 using EVENT::LCCollection, EVENT::ReconstructedParticle;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting. It compiles successfully in the CI workflows. But maybe that is only true for the limited number of compilers that we use there. Where did you test this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This valid c++17 code that gcc 6 does not seem to support. Do we still need gcc6 to work here ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andresailer has to decide if he still wants that pipeline. But in my opinion it's easier to fix this and keep the current system running.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we moving to c++17 for iLCSoft?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we did a long time ago ? At least we are using it for our iLCSoft release > 15 months or so ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have missed that memo 😃

Probably should move to base on the nightly key4hep installation for the CI, which includes the iLCSoft packages

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we stop the old nightly CI in this project?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants