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

EMTF three fixes to unpacking and emulation bugs #23136

Merged
merged 6 commits into from May 8, 2018

Conversation

abrinke1
Copy link
Contributor

@abrinke1 abrinke1 commented May 3, 2018

3 commits** to fix 3 recently-encountered bugs in the EMTF unpacker and emulator code

  1. "EMTF unpacker fix, only check format bits, not empty bits"
  • Unpacker was verifying that empty fields in EMTF payload read out 0s
  • Problematic when adding new information to payload
  • Caused unpacker to skip events in run 315623 when CPPF info was added to payload
  • @jiafulow , @dinyar
  1. "EMTF emulator fix, discard CSC LCTs with out-of-range strip or wire"
  • Email chain "Paused PromptReco jobs run 315107"
  • Emulator was trying to look up phi coordinates for LCTs with out-of-range strips
  • Caused emulator to crash in rare cases with buggy input LCTs
  • @thomreis , @fabiocos , @dmitrijus
  1. "EMTF unpacker fix, firmware version based CSC LCT chamber number"

Commit (2) will presumably be important for any RECO sequence involving the EMTF emulator, e.g. Offline DQM. Commit (1) will be important for any sequence involving the EMTF unpacker in run 315623, and future runs after we update the EMTF firmware to write out the CPPF link status. So both of these at least need to go to Tier-0, I guess. @rekovic any thoughts on the proper procedure here?

Cheers,
Andrew

** Accidentally also had some commits trying to fix EMTF O2O issues, reverted. No changes to those files.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2018

A new Pull Request was created by @abrinke1 for master.

It involves the following packages:

DataFormats/L1TMuon
EventFilter/L1TRawToDigi
L1Trigger/L1TMuonEndCap

@nsmith-, @rekovic, @cmsbuild, @thomreis can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @rovere, @kreczko, @thomreis this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@thomreis
Copy link
Contributor

thomreis commented May 3, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27784/console Started: 2018/05/03 23:14

@thomreis
Copy link
Contributor

thomreis commented May 3, 2018

Hi @abrinke1
could you make a 10_1_X backport PR as well of this one.
How have the unpacker changes been validated?

@thomreis
Copy link
Contributor

thomreis commented May 3, 2018

Do these unpacker upgrades also fix the EMTF track address mismatches with the uGMT unpacked inputs?

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23136/27784/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2722241
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2722057
  • DQMHistoTests: Total skipped: 183
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 124 log files, 14 edm output root files, 30 DQM output files

@fabiocos
Copy link
Contributor

fabiocos commented May 4, 2018

@abrinke1 was the fix for the problem in Tier0 tested to solve the crash in that job?

I see also a DataFormat change adding members, is this needed immediately?

@abrinke1
Copy link
Contributor Author

abrinke1 commented May 4, 2018

Hi @fabiocos ,
This is not strictly necessary, but this data format is used by exactly 0 consumers, other than the EMTF DQM, so there will be no downstream effects. That said, if you'd prefer I can comment these lines out for now.
Cheers, Andrew

@abrinke1
Copy link
Contributor Author

abrinke1 commented May 4, 2018

@thomreis I'm looking into the EMTF vs. uGMT disagreement, to see if I can add a quick fix for that as well. Then I'll do the unit test of the unpacking.
Cheers, Andrew

Update: no quick fix.

@abrinke1
Copy link
Contributor Author

abrinke1 commented May 7, 2018

Hi @rekovic , @thomreis , @dinyar , @fabiocos , all,

Can we proceed to testing this PR for possible inclusion in a patch release? The changes are quite trivial, and will fix various problems encountered recently at Tier-0 and in the DQM.

I'm still working on the unit tests for other systems as requested by @thomreis , but I'm having trouble running the L1Ntuple code locally on lxplus. In any case, I've verified that this functionality works for EMTF, and you can see in the code that the only change is to move a couple of functions that were duplicated in MuonUnpacker.h and IntermediateMuonUnpacker.h to Unpacker.h.

Thanks much,
Andrew

@rekovic
Copy link
Contributor

rekovic commented May 8, 2018

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 8, 2018

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@abrinke1
Copy link
Contributor Author

abrinke1 commented May 8, 2018

Back-port to CMSSW_10_1_X created: #23170

@fabiocos
Copy link
Contributor

fabiocos commented May 8, 2018

+1

@cmsbuild cmsbuild merged commit 379024d into cms-sw:master May 8, 2018
@abrinke1 abrinke1 deleted the EMTF_PR_2018_May_unp_emu_fixes branch September 18, 2018 12:38
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

5 participants