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

Revert NN tau variable mapping #1238

Conversation

HaarigerHarald
Copy link

Fixes #1237

@artlbv
Copy link

artlbv commented Apr 3, 2024

I tested it and it worked fine.

Please merge this ASAP as others can't use the IB.. @aloeliger @epalencia

@aloeliger aloeliger added Phase-2 Pertains to phase-2 development Bug fix For minor bug fixing PRs labels Apr 3, 2024
@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/L1TMuon/src/MicroGMTConfiguration.cc

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

@triggerDoctor
Copy link

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

This PR passes available unit tests!

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

@triggerDoctor
Copy link

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

I found a non-zero return code running the relval workflows for this PR!| Info | Value |
|:---------:|:------------:|
|return code|1|
|command|eval scramv1 runtime -sh && runTheMatrix.py --what upgrade -l 26834.78|

@aloeliger
Copy link

Hmm. The script output is wrong, but the return code is wrong. Let me check.

@aloeliger
Copy link

HLT overlap. Not this PR's issue.

@aloeliger
Copy link

@artlbv I am merging this PR.

@HaarigerHarald Could you please open a PR for this to CMSSW as soon as practical?

@aloeliger aloeliger merged commit c85ebef into cms-l1t-offline:phase2-l1t-integration-14_0_0_pre3 Apr 3, 2024
@artlbv
Copy link

artlbv commented Apr 3, 2024

thanks @aloeliger

on

@HaarigerHarald Could you please open a PR for this to CMSSW as soon as practical?

this should be really included in the same PR as #1225 (comment) so more for @Duchstf

@Duchstf
Copy link

Duchstf commented Apr 3, 2024

@artlbv Hello, since it was #1225 was merged, how should I proceed? do I need to create a PR?

@artlbv
Copy link

artlbv commented Apr 3, 2024

@Duchstf actually you probably should not do anything.. the P2GT emulator has not been updated in central yet. the PR is on hold: #1207

@aloeliger
Copy link

aloeliger commented Apr 3, 2024

I really don't want to lose either of these PRs to forgetfulness because we were all so busy with other phase 2 things. We should either:

  1. Revert this and Correlator PuppiTauNN interface with GT.  #1225 and immediately reopen a revert-revert to remind us that this is going to go in
  2. Merge this and Correlator PuppiTauNN interface with GT.  #1225 in a mini-update to CMSSW immediately.

This fork is only a convenience. Getting it in CMSSW is what actually matters.

@artlbv
Copy link

artlbv commented Apr 3, 2024

  1. is the way to go..
  2. will break the current P2GT emulator as it parses the Tau "isolation" for the ID: https://github.com/cms-sw/cmssw/blob/CMSSW_14_0_0_pre3/L1Trigger/Phase2L1GT/python/l1tGTMenu_lepSeeds_cff.py#L395 and fixing this will require the above P2GT PR in CMSSW

@HaarigerHarald
Copy link
Author

Why not open a PR to central that basically includes both #1225 and #1238 ?

@aloeliger
Copy link

@HaarigerHarald @Duchstf Ping again on opening a PR to CMSSW combining #1225 and #1238. Please do not lose these changes.

@HaarigerHarald
Copy link
Author

HaarigerHarald commented Apr 9, 2024

@Duchstf pls do it as it mostly concerns stuff in the correlator package. You can cherry-pick my commit into your branch and then just open a PR from it.

Edit: I opened a PR now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix For minor bug fixing PRs Phase-2 Pertains to phase-2 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants