Skip to content

Update to PuppiTau with pT calibration [13_3_0_pre3] [Merged in central CMSSW]#1202

Closed
Duchstf wants to merge 3 commits intocms-l1t-offline:phase2-l1t-integration-13_3_0_pre3from
Duchstf:puppitau_pt_13_3_0_pre3
Closed

Update to PuppiTau with pT calibration [13_3_0_pre3] [Merged in central CMSSW]#1202
Duchstf wants to merge 3 commits intocms-l1t-offline:phase2-l1t-integration-13_3_0_pre3from
Duchstf:puppitau_pt_13_3_0_pre3

Conversation

@Duchstf
Copy link

@Duchstf Duchstf commented Jan 29, 2024

PR description:

Update to PuppiTauNN with pT calibrations, making the PR here as recommended as well, for more details please visit: cms-sw#43639

The performance is documented in this talk here: https://indico.cern.ch/event/1321399/#17-phase-2-nn-puppi-taus

All codes compile, passed tests in central CMSSW CI, and we verified that the outputs are the same as the NN we implemented in Python (by @andrzejnovak):

image

l1ct::PFTau.passLooseNN() is again what people should use for studies.

@Duchstf Duchstf marked this pull request as ready for review January 29, 2024 12:41
@epalencia
Copy link

@Duchstf, this PR has 30 modified files while the one in master has 29, is it because of the different releases?

@Duchstf
Copy link
Author

Duchstf commented Jan 29, 2024

@epalencia oh no, I actually added a minor correction to the working point of the NN in this file:

https://github.com/cms-l1t-offline/cmssw/pull/1202/files#diff-1194e2aafd6ecf87da700631ba27cb1f104228e8a16370ce256a3971c49d25e1

Otherwise everything stay the same, I'll make another PR correcting this in the central PR.

@epalencia
Copy link

Would it make sense to remove that from here so we have a 1 to 1 correspondence?

@Duchstf
Copy link
Author

Duchstf commented Jan 29, 2024

@epalencia I think it would make more sense to add the same changes to central CMSSW.

@epalencia
Copy link

Yes, that for sure. We will have 2 PRs in master, that is why I thought having 2 PRs in the IB might make things more straight forward. I'll not insist, it is not so important.

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -j 8

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-checks && scram b -k -j 8 code-checks

I found no issues with the headers!

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 check-headers

@triggerDoctor
Copy link

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

  • L1Trigger/L1TTrackMatch/plugins/L1TrackJetClustering.h

Please run scram b code-format to auto-apply code formatting

Info Value
return code 0
command eval scramv1 runtime -sh && scram b -k -j 8 code-format-all

@epalencia
Copy link

@Duchstf , could you open a PR in master for the correction to the working point of the NN implemented here?

@aloeliger aloeliger changed the base branch from phase2-l1t-integration-13_3_0_pre3 to phase2-l1t-integration-14_0_0_pre3 February 5, 2024 15:45
@EmyrClement
Copy link

Can I check what we should do here? I believe that as the equivalent PR to master is merged and available in the new base branch for this PR, the only change that is now required is a few lines of changes to update the working point, in both the IB and in master?

@epalencia
Copy link

I think that is correct. Only this commit ab00ed9 is needed in this PR and in another one to CMSSW. The other two are already merged in master and included in this new 14_0_0_pre3 branch

@EmyrClement
Copy link

@Duchstf in that case, can you either rebase this PR to the new base branch, which should just effectively leave the commit mentioned by Enrique? Or if it's easier, open a new PR with just the one commit that updated the working point?

@epalencia
Copy link

@Duchstf , could you open a PR in master for the correction to the working point of the NN implemented here?

@aloeliger aloeliger added the Needs Rebase PR should be rebased to newer branch label Feb 13, 2024
@aloeliger aloeliger changed the base branch from phase2-l1t-integration-14_0_0_pre3 to phase2-l1t-integration-13_3_0_pre3 February 13, 2024 13:06
@aloeliger aloeliger added Phase-2 Pertains to phase-2 development Emulator Development Emulator development PR Configuration labels Feb 13, 2024
@aloeliger
Copy link

Hi @Duchstf, I'm going around starting to figure out how to update all PRs to our new IB

For this PR I would recommend:

  1. First, make a backup of this branch and it's commits pushed to github under a different name
  2. checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
  3. Get a copy of this branch locally via git cms-checkout-topic -u Duchstf:puppitau_pt_13_3_0_pre3
  4. Perform an interactive rebase via git rebase from-CMSSW_14_0_0_pre3 --interactive
  5. In the interactive rebase, please drop all commits that were not originally part of this pull request. In my tests, there seemed to be some earlier commits from track trigger commits that get caught up in this somehow?
  6. You can either then push the resulting branch to the current PR branch, or make a new one and make a PR from that branch to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3

Alternatively, because you have relatively few commits, you could just cherry-pick commits by doing something like:

  1. First, make a backup of this branch and it's commits pushed to github under a different name
  2. checkout CMSSW_14_0_0_pre3 via cmsrel CMSSW_14_0_0_pre3 && cd CMSSW_14_0_0_pre3/src/ && cmsenv && git cms-init
  3. Fetch your changes git fetch my-cmssw
  4. Cherry pick the 4 necessary commits via git cherry-pick, the 3 commands needed (in order) I think are: git cherry-pick ab0960a0f5e22f844e1d6d04a5dd96a7d3d21fed git cherry-pick e1128b41429a4a8496e6a89aa405ca7d2a3b4098 and git cherry-pick ab00ed9eeda5b3c077c72e5d5181eb6dbb699fc7
  5. You can either then push the resulting branch to the current PR branch, or make a new one and make a PR from that branch to cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3

@EmyrClement
Copy link

@Duchstf can this PR be closed now? Effectively the only remaining change introduced by this PR is implemented in #1214 (updating the WP)

@Duchstf Duchstf closed this Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Configuration Emulator Development Emulator development PR Needs Rebase PR should be rebased to newer branch Phase-2 Pertains to phase-2 development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments