Navigation Menu

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

[L1TMuonBarrel] KMTF fixes on propagation coefficients (CMSSW_11_2_X) #33281

Conversation

Tyler-Lam
Copy link
Contributor

PR description:

New version of KMTF algorithm as discussed in https://indico.cern.ch/event/1012620/contributions/4250430/attachments/2200192/3721090/Run3_KMTF.pdf

PR validation:

Nominal tests with just KMTF algorithm. No core changes, just incremental changes to KMTF

if this PR is a backport please specify the original PR and why you need to backport that PR:

Backport of #33078 , requested by the L1 Trigger group

@rekovic copied from #33078 - the LUTs in L1Trigger/L1TMuon/data/bmtf_luts/kalmanLUTs.root are usually in the https://github.com/cms-l1t-offline/L1Trigger-L1TMuon.git repository. They have to be moved but I do not have access

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Tyler-Lam for CMSSW_11_2_X.

It involves the following packages:

L1Trigger/L1TMuon
L1Trigger/L1TMuonBarrel

@cmsbuild, @rekovic, @cecilecaillol can you please review it and eventually sign? Thanks.
@dinyar, @Martin-Grunewald, @thomreis this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@jfernan2
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6bc68a/13812/summary.html
COMMIT: 1320360
CMSSW: CMSSW_11_2_X_2021-03-28-0000/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33281/13812/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2527501
  • DQMHistoTests: Total failures: 437
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2527042
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 151 log files, 37 edm output root files, 36 DQM output files
  • TriggerResults: found differences in 2 / 35 workflows

@cecilecaillol
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_11_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_11_3_X is complete. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@smuzaffar
Copy link
Contributor

hold
please see the issue #33303 . @Tyler-Lam please cleanup this PR and remove the root file. Please remove the file from the history so that we do not get it in repo. I have open cms-data/L1Trigger-L1TMuon#20 which can be integrated in 11.2.X to include your new root file

@cmsbuild
Copy link
Contributor

Pull request has been put on hold by @smuzaffar
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@smuzaffar
Copy link
Contributor

smuzaffar commented Mar 31, 2021

test parameters:

@smuzaffar
Copy link
Contributor

@Tyler-Lam , cms-sw/cmsdist#6773 should include the updated root file

@rekovic
Copy link
Contributor

rekovic commented Mar 31, 2021

So we should remove the root data file from 11_3_X merged in #33078. Please open a new 11_3_X PR that removes it.
This PR (11_2_X) can simply be updated.

@smuzaffar
Copy link
Contributor

@rekovic , for 11.3.X , I have already opened the PR #33304 .
For 11.2.X, yes, please update this PR . I would suggest to delete the root file and force push changes so that the root file does not appear in the history

@cmsbuild
Copy link
Contributor

Pull request #33281 was updated. @cmsbuild, @rekovic, @cecilecaillol can you please check and sign again.

@smuzaffar
Copy link
Contributor

smuzaffar commented Mar 31, 2021

@Tyler-Lam , pushing a new commit to remove file is fine but in order to reduce the 11.2.X branch size can you please locally squash the two commits https://www.internalpointers.com/post/squash-commits-into-one-git . This way the binary root file will not be part of 11.2.X branch history and will keep its size small.

@Tyler-Lam Tyler-Lam force-pushed the kmtf-fixed-propagation-11_2_X_2021-03-26-1100 branch from 9ae9b59 to d2df0ae Compare March 31, 2021 17:37
@cmsbuild
Copy link
Contributor

Pull request #33281 was updated. @cmsbuild, @rekovic, @cecilecaillol can you please check and sign again.

@smuzaffar
Copy link
Contributor

thanks a lot @Tyler-Lam

@smuzaffar
Copy link
Contributor

please test

@smuzaffar
Copy link
Contributor

unhold

@cmsbuild cmsbuild removed the hold label Mar 31, 2021
@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6bc68a/13903/summary.html
COMMIT: d2df0ae
CMSSW: CMSSW_11_2_X_2021-03-31-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33281/13903/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 36
  • DQMHistoTests: Total histograms compared: 2527501
  • DQMHistoTests: Total failures: 439
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2527040
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 151 log files, 37 edm output root files, 36 DQM output files
  • TriggerResults: found differences in 2 / 35 workflows

@cecilecaillol
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2021

This pull request is fully signed and it will be integrated in one of the next CMSSW_11_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_11_3_X is complete. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato silviodonato changed the title KMTF fixes on propagation coefficients (CMSSW_11_2_X) [L1TMuonBarrel] KMTF fixes on propagation coefficients (CMSSW_11_2_X) Apr 2, 2021
@silviodonato
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit e6aa717 into cms-sw:CMSSW_11_2_X Apr 2, 2021
@silviodonato
Copy link
Contributor

@cms-sw/externals-l2 @qliphy
I've made a mistake in the morning IB
11_2_X: cms-sw/cmsdist#6773 needed for #33281
11_3_X: cms-sw/cmsdist#6772 needed for #33304

I merged cms-sw/cmsdist#6773 + #33304 in the morning IB, so I think we will get error in CMSSW_11_2_X_2021-04-02-1100. It should be fixed in CMSSW_11_2_X_2021-04-02-2300

@smuzaffar
Copy link
Contributor

I am starting 11.2.X B now ( just to have a clean IB , in case someone wants to use it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants