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

TrackState Covariance matrix #107

Closed
andresailer opened this issue Feb 11, 2021 · 6 comments · Fixed by #108
Closed

TrackState Covariance matrix #107

andresailer opened this issue Feb 11, 2021 · 6 comments · Fixed by #108

Comments

@andresailer
Copy link
Collaborator

Replacing: key4hep/k4LCIOReader#8

At the moment the LCIO covariance matrix is lower triangle, while the one in EDM4hep is the upper triangle.

http://lcio.desy.de/v02-09/doc/doxygen_api/html/classEVENT_1_1TrackState.html#a667b8774ce1b0f42dbae11a59dc7e09b

- std::array<float, 15> covMatrix // upper triangular covariance matrix of the track parameters. the order of parameters is d0, phi, omega, z0, tan(lambda).

It was decided to change the edm4hep one to lower triangle as well.

Is there any sane way to prevent existing places where this conversion was done after reading the documentation to not silently break?

@clementhelsens
Copy link
Contributor

Hi @andresailer , thanks for deciding :)
As far as I know, this is the only place where the covariance matrix is filled.

@andresailer
Copy link
Collaborator Author

I didn't decide this: https://indico.cern.ch/event/1000558/

@clementhelsens
Copy link
Contributor

sorry, I misread you comment and missed the "t was" :)
Anyway, this part of the code

https://github.com/key4hep/k4SimDelphes/blob/8561d3fc39b2accc99c62a561090421fad3f1e8e/converter/src/DelphesEDM4HepConverter.cc#L488-L493

will have to be changed once we have properly patched Delphes to get the full matrix.

@andresailer
Copy link
Collaborator Author

I should have linked the minutes in the ticket from the start.

The delphes part can already be changed to assume lower triangle though. Still basically makes a mess that there will be files with one and some with a different interpretation.

@clementhelsens
Copy link
Contributor

For FCC-ee, I do not think this is problematic. I will anyway remove all the files we have been producing so far for a first clean production where I want to have the full matrix stored, so no show stopper from my side a least!

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 a pull request may close this issue.

2 participants