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

Update track fit EDM #58

Merged
merged 6 commits into from
Nov 2, 2023
Merged

Update track fit EDM #58

merged 6 commits into from
Nov 2, 2023

Conversation

osbornjd
Copy link
Contributor

Briefly, what does this PR introduce?

This introduces changes to the track fit and track state EDM objects to better conform to a single container that can deliver all of the needs of downstream reconstruction algorithms and (ultimately) physics analysis.

  1. Introduces a 6D covariance matrix object that is really the upper triangle of a full 6x6 covariance, for storage purposes.
  2. Moves some "track level" quantities out from edm4eic::Trajectory and into edm4eic::Track
  3. Revamps the edm4eic::TrackParameters object, which will need to contain the complete covariance to swap between edm4eic and Acts::EDM. Also adds a surface type as bound track parameters make no sense without an associated surface.
  4. Adds a full covariance, position, and PID hypothesis to edm4eic::Track

The resulting track object contains the set of track parameters at the vertex, some criteria to evaluate the track fit (e.g. those contained in the one to one relation to edm4eic::Trajectory), and all track states associated to the fit (also still in edm4eic::Trajectory).

The track parameters at the vertex do need to be stored separately (e.g. not as another uniquely defined track state), for a technical reason. They are going to (1) be most frequently used in global coordinates, which the track parameters are not in and (2) are associated to a PerigeeSurface, which is transient unlike the surfaces associated with the actual geometry. Without knowledge of the surface, a set of BoundTrackParameters do not have a well defined global definition.

Opening this PR now as it will likely have nontrivial downstream consequences, so comments welcome.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

It introduces breaking changes to EICrecon. Currently there are 3 containers produced by the CKF in EICrecon - the aim of this PR is to reduce this to a single container. This will in turn change other downstream reconstruction algorithms that rely on the CKF output as they will have to access this changed structure.

Does this PR change default behavior?

No

@osbornjd osbornjd requested a review from a team as a code owner October 19, 2023 13:52
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Outdated Show resolved Hide resolved
edm4eic.yaml Show resolved Hide resolved
@wdconinc
Copy link
Contributor

wdconinc commented Nov 1, 2023

  1. Propagate the Cov6f interface to make other Cov2,3f components identical in API, even if possible regression.
    • this can be addressed in a separate PR
  2. Vector3f for track vertex position is not how we can constrain tracks, and I'd want that in the Vertex
    • Ideally Track is what comes out of CKFTracking, and we cannot add a vertex later through IVF. So what would this be filled with?
  3. Cov6f for track position and momentum in cartesian components is unlikely to be calculable.
    • How do we get these cross correlations? Does Acts give those as output from IVF? Does it make sense to have these in cartesian vs spherical coordinates for momentum? Or will we provide a Jacobian to get from one to the other? I like the idea to allow full covariance, but I don't know if this is the coordinate system that makes most sense.

@osbornjd
Copy link
Contributor Author

osbornjd commented Nov 1, 2023

  1. Propagate the Cov6f interface to make other Cov2,3f components identical in API, even if possible regression.

    * this can be addressed in a separate PR
    

That makes sense.

2. Vector3f for track vertex position is not how we can constrain tracks, and I'd want that in the Vertex
   
   * Ideally Track is what comes out of CKFTracking, and we cannot add a vertex later through IVF. So what would this be filled with?

This is not a vertex position, this is the track position of closest approach to the vertex (or some, in the case of Acts, PerigeeSurface which is right now by default set to (0,0,0) in the CKF). A set of track parameters can't be uniquely defined without a position.

3. Cov6f for track position and momentum in cartesian components is unlikely to be calculable.
   
   * How do we get these cross correlations? Does Acts give those as output from IVF? Does it make sense to have these in cartesian vs spherical coordinates for momentum? Or will we provide a Jacobian to get from one to the other? I like the idea to allow full covariance, but I don't know if this is the coordinate system that makes most sense.

It is calculable via jacobians that relate the two bases, there are transforms that do this that you can basically copy from the CovarianceEngine within Acts. I'll emphasize that this covariance has nothing to do with a vertex, it is on the track position itself WRT a PerigeeSurface. My personal opinion is that it does not make sense to store a covariance in local coordinates because it has no physical meaning without an associated surface, and the track parameters at a PerigeeSurface can change depending on where they are propagated (e.g. if they were determined at a primary vs. secondary vertex or something of this nature). Analyzers will, unless I am mistaken, work in global coordinates and the covariance is needed to determine things like the uncertainty on the DCA. We are either going to have to provide the parameters at the perigee surface in global coordinates, or provide them in local coordinates + a surface and additionally provide analyzers with a standard way to transform to global

@wdconinc
Copy link
Contributor

wdconinc commented Nov 2, 2023

Sounds good to me. Up to @sly2j to click the merge button.

@osbornjd
Copy link
Contributor Author

osbornjd commented Nov 2, 2023

So for my own understanding - this gets merged first and then we make the corresponding changes to EICrecon that will reflect these changes? I'm pretty sure merging this will break the nightly build

@wdconinc
Copy link
Contributor

wdconinc commented Nov 2, 2023

The nightly build in eic-shell doesn't pull in the main branch of just anything, only epic and eicrecon. So merging this doesn't mean it gets into eic-shell. We first need to cut a release here and put it explicitly in eic-shell.

@osbornjd
Copy link
Contributor Author

osbornjd commented Nov 2, 2023

Thanks for the clarification, I naively assumed all the repos got built together rather than some being only released on versions and some not. That makes sense for repos that don't change as frequently as others

@wdconinc
Copy link
Contributor

wdconinc commented Nov 2, 2023

There's the eternal tension between only putting releases into the environments rolled out to users by default, and putting only rolling main versions into the nightly. I wish we could have a nightly that's just main only but it's not workable and leads to too much breakage. So we've settled on epic and eicrecon main only... And there's pressure even here, in particular when changing acts versions in non-compatible ways wrt eicrecon.

@osbornjd osbornjd merged commit 6904444 into main Nov 2, 2023
3 checks passed
@osbornjd osbornjd deleted the trackfit_edm branch November 2, 2023 15:36
@ShujieL ShujieL mentioned this pull request Nov 9, 2023
5 tasks
wdconinc added a commit to eic/algorithms that referenced this pull request Jan 20, 2024
After eic/EDM4eic#58, there is no more `charge` field in edm4eic::TrackParameters, so we have to get this from `qOverP`, but this is backwards compatible.
wdconinc added a commit to eic/algorithms that referenced this pull request Jan 20, 2024
After eic/EDM4eic#58, there is no more `charge` field in edm4eic::TrackParameters, so we have to get this from `qOverP`, but this is backwards compatible.
wdconinc added a commit that referenced this pull request Jan 20, 2024
### Briefly, what does this PR introduce?
Due to incompatible changes in #58 the next version will be 5.0.0.
github-merge-queue bot pushed a commit to eic/EICrecon that referenced this pull request Jan 24, 2024
### Briefly, what does this PR introduce?
This adds forward support for edm4eic version 5, which includes the
changes in eic/EDM4eic#58.

### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [x] New feature (issue, follow-up on
eic/EDM4eic#58)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
Yes, some fields not written as before since CKFTracking does not yet
write edm4eic::Track collection.
ShujieL added a commit that referenced this pull request Jan 26, 2024
### Briefly, what does this PR introduce?
Move measurements from Track (which is introduced in
#58) back to Trajectory, and add back
outliers.


### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [x ] Other: partially revert a change

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [x ] Changes have been communicated to collaborators. -- This change
is communicated to Joe Osborn.

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
CKFtracking.cc needs to be updated to take the new data structure.

### Does this PR change default behavior?

---------

Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants