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

fix(gx2f): use temporary track container #3247

Merged
merged 3 commits into from
Jun 8, 2024

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Jun 4, 2024

Use a temporary track container for the propagation during the updates, to ensure the original track container stays untouched.

Until now, we were clearing the original container with trackContainer.clear(). This is fine, until we have more tracks in our container.

blocked by:

edit:
After #3248 the physmon does not change, since we switched from 2 particles/event to 1. Therefore, we also have only one track/container.

Before we could see the effect stronger, like in the graphic. However, I don't see a reason to change physmon to catch this.
image

@AJPfleger AJPfleger added this to the next milestone Jun 4, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Track Fitting labels Jun 4, 2024
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 47.65%. Comparing base (c2ee464) to head (3b71d89).

Current head 3b71d89 differs from pull request most recent head 360c566

Please upload reports for the commit 360c566 to get more accurate results.

Files Patch % Lines
...nclude/Acts/TrackFitting/GlobalChiSquareFitter.hpp 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3247      +/-   ##
==========================================
- Coverage   47.65%   47.65%   -0.01%     
==========================================
  Files         509      507       -2     
  Lines       29423    29207     -216     
  Branches    14132    14009     -123     
==========================================
- Hits        14023    13920     -103     
+ Misses       5285     5264      -21     
+ Partials    10115    10023      -92     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AJPfleger AJPfleger added the 🛑 blocked This item is blocked by another item label Jun 5, 2024
@AJPfleger AJPfleger removed the 🛑 blocked This item is blocked by another item label Jun 7, 2024
@kodiakhq kodiakhq bot merged commit 75ef913 into acts-project:main Jun 8, 2024
54 checks passed
@AJPfleger AJPfleger deleted the temp-track-container branch June 8, 2024 08:55
@github-actions github-actions bot removed the automerge label Jun 8, 2024
kodiakhq bot pushed a commit that referenced this pull request Jun 11, 2024
This PR allows to fully support measurements with non-diagonal covariance matrices.

## How?
Loop over track states after the propagation extract the needed information for the update. Before we used the `collector()` inside the actor for this task. The collector was too confining, since we had to convert all n-dimensional measurements into n 1-dimensional measurements, removing all non-diagonal information from their covariances.

Blocked by:
- #3247
- #3248
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jun 14, 2024
This PR allows to fully support measurements with non-diagonal covariance matrices.

## How?
Loop over track states after the propagation extract the needed information for the update. Before we used the `collector()` inside the actor for this task. The collector was too confining, since we had to convert all n-dimensional measurements into n 1-dimensional measurements, removing all non-diagonal information from their covariances.

Blocked by:
- acts-project#3247
- acts-project#3248
@andiwand andiwand modified the milestones: next, v35.2.0 Jun 16, 2024
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
Use a temporary track container for the propagation during the updates, to ensure the original track container stays untouched.

Until now, we were clearing the original container with `trackContainer.clear()`. This is fine, until we have more tracks in our container.

blocked by:
- acts-project#3248

edit:
After acts-project#3248 the physmon does not change, since we switched from 2 particles/event to 1. Therefore, we also have only one track/container.

Before we could see the effect stronger, like in the graphic. However, I don't see a reason to change physmon to catch this.
![image](https://github.com/acts-project/acts/assets/70842573/55eb47b2-e268-4a01-a618-8379d36bae49)
Matthewharri pushed a commit to Matthewharri/acts that referenced this pull request Jun 18, 2024
This PR allows to fully support measurements with non-diagonal covariance matrices.

## How?
Loop over track states after the propagation extract the needed information for the update. Before we used the `collector()` inside the actor for this task. The collector was too confining, since we had to convert all n-dimensional measurements into n 1-dimensional measurements, removing all non-diagonal information from their covariances.

Blocked by:
- acts-project#3247
- acts-project#3248
timadye pushed a commit to andiwand/acts that referenced this pull request Jun 27, 2024
This PR allows to fully support measurements with non-diagonal covariance matrices.

## How?
Loop over track states after the propagation extract the needed information for the update. Before we used the `collector()` inside the actor for this task. The collector was too confining, since we had to convert all n-dimensional measurements into n 1-dimensional measurements, removing all non-diagonal information from their covariances.

Blocked by:
- acts-project#3247
- acts-project#3248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Track Fitting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants