-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 2017 PR #6 - Emulator #20286
EMTF 2017 PR #6 - Emulator #20286
Conversation
The code-checks are being triggered in jenkins. |
A new Pull Request was created by @abrinke1 for master. It involves the following packages: CondCore/L1TPlugins @ghellwig, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @rekovic, @franzoni, @ggovi, @vanbesien, @mulhearn, @lpernie can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
*** N.B. on the O2O/CondDB changes *** You may notice that one of the commits involves renaming some existing files. This was done to remedy a disastrous situation that was created unintentionally a year ago, where some files used the "L1TMuonEndCapParams" convention, and some used "L1TMuonEndcapParams". In fact, the situation was so confused that it was necessary to have duplicate DataRecord files, one set with "C", and one set with "c", for the code to compile and run without crashing: http://cmslxr.fnal.gov/search?_filestring=L1TMuonEndCapParams&_string=&_casesensitive=1 This had not caused too many practical issues up to this point because the O2O/CondDB conditions were actually used by the EMTF emulator. But this emulator does, and we will most likely need to port more EMTF functionalities into O2O/CondDB in the future. This means that the code needs to be usable / developable by someone other than the person who wrote it. Right now (before this PR), that is not the case. And if we add the new O2O/CondDB pieces needed by the 2017 emulator without resolving the naming confusion, the situation will only get much worse. It's a bit of pain in tracking file changes now, in exchange for long-term sanity. Even so, because of the Record name in the existing Global Tags, we need to keep two duplicate files and one extra line of code for things to run without crashing, for the moment: Once this PR is in, we can use it to create a new GT, at which point those lines/files can be deleted, and the code will assume a state of serene and beautiful simplicity. Or something like that :) |
The code-checks are being triggered in jenkins. |
Pull request #20286 was updated. @ghellwig, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @rekovic, @franzoni, @ggovi, @vanbesien, @mulhearn, @lpernie can you please check and sign again. |
+code-checks |
+code-checks |
please test |
The tests are being triggered in jenkins. |
-1 Tested at: 44af81d The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests ClangBuild
I found errors in the following unit tests: ---> test TestTrackTools had ERRORS
I found a compilation error while trying to compile with clang: >> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/MP7BufferDumpToRaw.cc 2 warnings generated. >> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/AMCDumpToRaw.cc 2 warnings generated. >> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/L1TDigiToRaw.cc /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/L1Trigger/L1TMuonEndCap/test/tools/MakePtLUT.cc:125:19: error: comparison of integers of different signs: 'PtLUTWriter::address_t' (aka 'unsigned long') and 'int' [-Werror,-Wsign-compare] for ( ; address < abs(num_ * (PTLUT_SIZE / denom_)); ++address) { ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 warnings generated. 2 warnings generated. >> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_3_X_2017-08-29-1100/src/EventFilter/L1TRawToDigi/plugins/L1TCaloTowersFilter.cc The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Hi Andrew, For the Unit Tests failure, the following should fix it: diff --git a/L1TMuonEndCap/test/BuildFile.xml b/L1TMuonEndCap/test/BuildFile.xml
index cbf172a..9a10c30 100644
--- a/L1TMuonEndCap/test/BuildFile.xml
+++ b/L1TMuonEndCap/test/BuildFile.xml
@@ -1,9 +1,4 @@
<environment>
- <bin name="TestL1TriggerL1TMuonEndCap" file="unittests/TestDriver.cpp">
- <flags TEST_RUNNER_ARGS=" /bin/bash L1Trigger/L1TMuonEndCap/test/unittests runtests.sh"/>
- <use name="FWCore/Utilities"/>
- </bin>
-
<bin name="TestPhiMemoryImage" file="unittests/TestPhiMemoryImage.cpp">
<use name="L1Trigger/L1TMuonEndCap"/>
<use name="cppunit"/>
diff --git a/L1TMuonEndCap/test/unittests/TestTrackTools.cpp b/L1TMuonEndCap/test/unittests/TestTrackTools.cpp
index 8b4248a..19962c0 100644
--- a/L1TMuonEndCap/test/unittests/TestTrackTools.cpp
+++ b/L1TMuonEndCap/test/unittests/TestTrackTools.cpp
@@ -53,9 +53,9 @@ void TestTrackTools::test_theta()
{
for (int i = 0; i < 1801; ++i) {
double theta = 0. + 0.1 * static_cast<double>(i); // theta=[0,180,step=0.1]
- int endcap = (theta >= 90.) ? 2 : 1;
+ int endcap = (theta >= 90.) ? -1 : 1;
double eps = 0.28515625;
- CPPUNIT_ASSERT_DOUBLES_EQUAL((endcap == 2 ? (180. - theta) : theta), calc_theta_deg_from_int(calc_theta_int(theta, endcap)), eps);
+ CPPUNIT_ASSERT_DOUBLES_EQUAL((endcap == -1 ? (180. - theta) : theta), calc_theta_deg_from_int(calc_theta_int(theta, endcap)), eps);
}
}
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@rekovic @davidlange6 @dmitrijus @Dr15Jones @smuzaffar |
+1 |
To facilitate further review of the PR, here is the list of files that are simply moved from directory L1Trigger/L1TMuonEndCap/src/bdt/Forest.cc |
(thanks @abrinke1 ) |
@abrinke1 |
@rekovic Do you have a suggestion on how to do this? I started cherry-picking my commits into a 92X release, but on the second commit, I got this message: git cherry-pick 33fd87f Is there a standard way to back-port four separate PRs into an earlier CMSSW? Also, any news on the code review? I would assume the deadline for MC production is pretty soon, and I doubt the analyses want last year's trigger in their MC. |
For the record, a [92x] rebase is available in #20768. |
merge |
Hi,
The above error usually points to the payload being in the DB, but not for the run in question! |
Hi @Martin-Grunewald |
My tests used the IB CMSSW_9_4_X_2017-10-08-2300 plus PR #20577. It looks like it this PR and #20577 step on each other, as the error is not present if I use just CMSSW_9_4_X_2017-10-08-2300 |
Indeed, #20577 no longer passes tests. So I suppose a fix needs to be added there! |
4th and last part of set of PRs following PR #19741 , #19743 , and #20006
PR #19744 is no longer necessary
PR #19928 (CondDB and O2O) will no longer be necessary
Author comments: This PR is huge. This was inevitable, and unavoidable. The 2016 emulator (with data formats) had 143 files. The 2017 emulator (with helper files for trigger primitives) has 126. So that's 269 out of the 295 total files changed. The remainder come from moving some files out of a deprecate folder (which have been, and continue to be in use), and changes for the new O2O/CondFormats files which are inseparable from the new emulator.**
I've tried to make it quite simple to track which changes happened where by grouping them in easy-to-understand commits:
https://github.com/abrinke1/cmssw/commits/EMTF_PR_2017_93X_part8c
Of course, it's still a lot of code to review. I suppose the first thing, though, is to figure out what the conflicts are that prevent this branch from being merged in its current state. It merges, compiles, and runs fine on CMSSW_9_3_0_pre4.
Best regards,
Andrew