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

Rearrange newKF chain #281

Merged
merged 3 commits into from
Jul 1, 2024
Merged

Conversation

cgsavard
Copy link

@cgsavard cgsavard commented Jun 18, 2024

PR description:

This PR removes the ProducerTT and ProducerAS modules and instead converts the KF tracks to TTTracks in the KFout module. This was necessary in order to fill the track quality MVA variables in the TTTracks which is calculated in the KFout. This also removes AnalyzerTT as a result.

old chain: KF -> TT -> AS -> KFout
new chain: KF -> KFout (conversion to tttracks done here)

PR validation:

The code checks have been run and I have checked that the results of the KFout module before and after the changes made match.

@cgsavard cgsavard changed the title Rearrange new kf l1tk Rearrange newKF chain Jun 19, 2024
@Chriisbrown
Copy link

image

The track MVA1 is looking off when running the L1TrackNtupleMaker with hybrid_newkf

@cgsavard
Copy link
Author

cgsavard commented Jun 28, 2024

The track MVA1 is looking off when running the L1TrackNtupleMaker with hybrid_newkf

This is expected since the chi2 variables are not correct. They are not /dof and so their values is larger than expected, and therefore we expect things to look more fake (will be fixed in a separate PR).

Copy link

@tschuh tschuh left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@cgsavard cgsavard merged commit 5cf9b27 into L1TK-dev-14_0_0_pre2 Jul 1, 2024
1 check passed
@cgsavard
Copy link
Author

cgsavard commented Jul 2, 2024

@tschuh @Chriisbrown @tomalin I attempted to merge but all of a sudden it looks like the merged commit no longer passes the git checks (it fails at setup_cmssw 5cf9b27). Can someone more knowledgable on the git checks advise on what should be done here? Should I revert the merge for now?

@tschuh
Copy link

tschuh commented Jul 2, 2024

Well, it says that it got successfully merged. Fresh checkout also compiles. I think everything is ok.

@tomalin
Copy link
Collaborator

tomalin commented Jul 18, 2024

The track MVA1 is looking off when running the L1TrackNtupleMaker with hybrid_newkf

This is expected since the chi2 variables are not correct. They are not /dof and so their values is larger than expected, and therefore we expect things to look more fake (will be fixed in a separate PR).

Anyone know why the chi2 variables are not correct with hybrid_newkf? Claire implies they are not normalized to dof. Why not?

@Chriisbrown
Copy link

The track MVA1 is looking off when running the L1TrackNtupleMaker with hybrid_newkf

This is expected since the chi2 variables are not correct. They are not /dof and so their values is larger than expected, and therefore we expect things to look more fake (will be fixed in a separate PR).

Anyone know why the chi2 variables are not correct with hybrid_newkf? Claire implies they are not normalized to dof. Why not?

When the firmware and emulation was originally written I was going off the digitization format of the twiki that doesn't mention /dof and has an older binning, the more recent interface document does give /dof and a revised binning so the emulation and firmware need to be updated to the /dof version. As previously to this PR the track word wasn't filled by the KF out and this non-dof version of the chi2 it didn't impact anything downstream, now that it does fill the trackword this needs fixing. It was decided that this would be a separate PR and that the chi2 values and MVA values will be wrong until that fix is done

dally96 pushed a commit that referenced this pull request Dec 3, 2024
* make tttracks in kfout

* remove ProducerTT, ProducerAS, AnalyzerAS, and fix conversion of kfout tracks

* fix stub indexing
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