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

WIP: Add Reconstructed TOF. #299

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

mirguest
Copy link
Contributor

@mirguest mirguest commented May 7, 2024

BEGINRELEASENOTES

  • Add Reconstructed TOF info

ENDRELEASENOTES

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.

Quick summary from the discussion that happened at the EDM4hep meeting (see the attached notes).

  • Do we need the 5 different path lengths and expected times for the 5 particle species?
    • Different track length / time for different species
    • Could create new tracks for the different species and link to that
  • Refitting (and storing) the track might be prohibitive due to size (and information duplication)
    • No clear best solution to this yet. Needs some further discussion

At this point in time it's not yet clear whether refitting and storing tracks for each particle species and then having a simpler RecTof is "better" than the current design, where there is the one canonical track that can then be linked to the current (more complicated) RecTof.

Some minor questions / comments also below.

Comment on lines +789 to +793
- float time [ns] // time measurement
- std::array<float, 5> timeExp [ns] // expected time for e(0),mu(1),pi(2),K(3),p(4)
- float sigma // time resolution
- std::array<float, 5> pathLength [mm] // length of flight for e(0),mu(1),pi(2),K(3),p(4)
- edm4hep::Vector3d position [mm] // extrapolated hit position
Copy link
Contributor

Choose a reason for hiding this comment

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

I would potentially re-order these to keep the particle species bits and the "measured" bits together, e.g.

  • time
  • sigma
  • position

sigma still needs a unit, which, I would assume is ps?

Is the extrapolated hit position necessary here? I assume you need it for the calculation of the track length, but do you still need it afterwards?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also propose sigma -> timeError, which is similar to other places where we have value and valueError

Copy link
Contributor

@m-fila m-fila May 7, 2024

Choose a reason for hiding this comment

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

How about renaming sigma to something like resolution
Sorry didn't see Andre's comment, timeError seems nice

Copy link
Collaborator

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.

In principle we also have the edm4hep::Quantity that bundles both:

EDM4hep/edm4hep.yaml

Lines 221 to 225 in a296139

edm4hep::Quantity:
Members:
- int16_t type // flag identifying how to interpret the quantity
- float value // value of the quantity
- float error // error on the value of the quantity

However, that also brings along another 16 free bits. Not sure if we want to go there.

- float time [ns] // time measurement
- std::array<float, 5> timeExp [ns] // expected time for e(0),mu(1),pi(2),K(3),p(4)
- float sigma // time resolution
- std::array<float, 5> pathLength [mm] // length of flight for e(0),mu(1),pi(2),K(3),p(4)
Copy link
Collaborator

@andresailer andresailer May 15, 2024

Choose a reason for hiding this comment

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

Add one "unknown/background/other" entry?

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.

4 participants