Skip to content

Comments

Kalman dev#842

Merged
pbutti merged 4 commits intomasterfrom
Kalman_dev
Apr 13, 2021
Merged

Kalman dev#842
pbutti merged 4 commits intomasterfrom
Kalman_dev

Conversation

@pbutti
Copy link
Contributor

@pbutti pbutti commented Apr 8, 2021

No description provided.

…. Also, improvements in refinement of already found tracks (e.g. dropping bad hits). Negative covariance still does happen, so more improvements are in the works. At least these changes prevent those tracks from becoming completely crazy.
…nal formula. The new formula clearly wasn't working.
@pbutti
Copy link
Contributor Author

pbutti commented Apr 13, 2021

Hi All,

Can people help reviewing this PR and approving it. From my side looks fine and I'd like to test this asap.

@normangraf
Copy link
Contributor

I'm trying to figure out how best to test this. Perhaps we can discuss it at either the analysis or recon meeting on 4/13.

@pbutti
Copy link
Contributor Author

pbutti commented Apr 13, 2021

Ok for recon meeting tomorrow then =)

Cinv.unsafe_set(i,j,1.0/Cinv.unsafe_get(i,j));
} else {
Cinv.unsafe_set(i, j, 0.);
SquareMatrix invrs = KalTrack.mToS(snP.helix.C).fastInvert();
Copy link
Collaborator

Choose a reason for hiding this comment

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

For future, this can easily be turned into a stream with a lambda instead. Don't think it's needed now, but wanted to note it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This applies to most for loops ...

@omar-moreno
Copy link
Collaborator

At some point, we should move to using a eigen or some other math library ... again, future cleanup.

Copy link
Collaborator

@cbravo135 cbravo135 left a comment

Choose a reason for hiding this comment

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

It appears distribution/pom.xml has not been updated yet to the 5.1 snapshot. Since git says this can be automerged, this small issue will automatically be dealt with. These changes are isolated to details of the KF tracking, which Robert explained during the reconstruction meeting. Testing will happen before we decide to run on any serious amount of data and additional changes can still be made going forward.

@pbutti pbutti merged commit 67ccc93 into master Apr 13, 2021
@pbutti
Copy link
Contributor Author

pbutti commented Apr 13, 2021

I merged it in. Since KF tracking is still in dev, more iterations will be done here for test this properly in the future

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.

5 participants