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
ME0Segment update for muon tdr samples #18127
Conversation
A new Pull Request was created by @nickmccoll (Nick McColl) for master. It involves the following packages: DataFormats/GEMRecHit @perrotta, @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @kpedro88, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
DataFormats/GEMRecHit/BuildFile.xml
Outdated
@@ -4,6 +4,8 @@ | |||
|
|||
<use name="DataFormats/GeometryVector"/> | |||
<use name="DataFormats/CSCRecHit"/> | |||
<use name="Geometry/GEMGeometry"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - DataFormats cannot depend on Geometry
@@ -5,6 +5,9 @@ | |||
*/ | |||
#include "FWCore/MessageLogger/interface/MessageLogger.h" | |||
#include "DataFormats/GEMRecHit/interface/ME0Segment.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - I think, the place for this class is in Geometry/GEMGeometry
DataFormats/GEMRecHit/BuildFile.xml
Outdated
@@ -4,6 +4,8 @@ | |||
|
|||
<use name="DataFormats/GeometryVector"/> | |||
<use name="DataFormats/CSCRecHit"/> | |||
<use name="Geometry/GEMGeometry"/> | |||
<use name="root"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - also, brining in root dependency here is not a good idea
@@ -5,6 +5,9 @@ | |||
*/ | |||
#include "FWCore/MessageLogger/interface/MessageLogger.h" | |||
#include "DataFormats/GEMRecHit/interface/ME0Segment.h" | |||
#include "Geometry/GEMGeometry/interface/ME0Chamber.h" | |||
#include "Geometry/GEMGeometry/interface/ME0Layer.h" | |||
#include <TVector2.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - isn't there a CMS equivalent for it in DataFormats/GeometryVector?
|
||
|
||
float deltaPhi() const { return theDeltaPhi; } | ||
static float computeDeltaPhi(const ME0Chamber * chamber, const LocalPoint& position, const LocalVector& direction ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - this function does not belong in ME0Segment, nor in DataFormats. I can see it is used in a plugin ME0SegmentAlgorithm. Shouldn't it be moved there? @slava77 - please, suggest a better place for it to avoid circular dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it in the ME0SegmentAlgorithm, if there is no problem that RecoMuonProducer depends on it. Otherwise I can duplicate the code and place it both in the global muon reconstruction and in the segment algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - which class represents RecoMuonProducer? Duplicating the code is not a good idea, nor introducing dependency on a plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianna It would be used under: RecoMuon/GlobalMuonProducer/src/GlobalMuonProducer.h and the related classes when building RecoMuons. Technically it should also be used in ME0SegmentMatcher.
I couldn't really think of a good place to put this function, and now that I think about it the ME0Segment isn't a very good place for it at all. I am open to any suggestion!
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll - would RecoMuon/GlobalTrackingTools be a good place for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the bending angle is something strictly related to the segment, then later to the matching and the muon ID. so the function can go into the algorithms that build the segment. why do you think it is not a good place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickmccoll Usually Reco depends on LocalReco and not vice versa. How about placing it in Geometry/GEMGeometry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianna That is a good idea, it naturally fits in as a ME0Chamber function. I updated the PR, and there are no new dependencies!
please test |
The tests are being triggered in jenkins. |
-1 Tested at: 548e308 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test runtestRecoEgammaElectronIdentification had ERRORS |
Comparison job queued. |
Looking at the unit tests, I see this as a cause of failure: pickRelValInputFiles WARNING : No RelVal file(s) found at all in datasets '/RelValTTbar_13/CMSSW_7_4_0_pre6-PU50ns_MCRUN2_74_V0-v*/GEN-SIM-RECO*' on SE 'T2_CH_CERN' |
@smuzaffar is this a file deletion issue, or just a temporary interruption? |
-1 Tested at: d74aaa1 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test testRecoMETMETProducers had ERRORS |
Comparison job queued. |
The error in the unit test is still there [1], but I can hardly believe it is caused by this PR [1]
---> test testRecoMETMETProducers had ERRORS ^^^^ End Test testRecoMETMETProducers ^^^^ |
try to rerun.
It may go away.
(the error has something to do with this ttbarForMetTests.root written
to a common test area which may be also used in parallel by another unit
test which cleans up the directory
…On 4/5/17 3:02 PM, perrotta wrote:
The error in the unit test is still there [1], but I can hardly believe
it is caused by this PR
[1]
* cmsRun
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_9_1_X_2017-04-04-1100/src/RecoMET/METProducers/test/recoMET_caloMet_cfg.py
05-Apr-2017 12:17:55 CEST Initiating request to open file
file:ttbarForMetTests.root
----- Begin Fatal Exception 05-Apr-2017 12:17:55
CEST-----------------------
An exception of category 'FileOpenError' occurred while
[0] Constructing the EventProcessor
[1] Constructing input source of type PoolSource
[2] Calling RootFileSequenceBase::initTheFile()
[3] Calling StorageFactory::open()
[4] Calling File::sysopen()
Exception Message:
Failed to open the file 'ttbarForMetTests.root'
Additional Info:
[a] Input file file:ttbarForMetTests.root could not be opened.
[b] open() failed with system error 'No such file or directory'
(error code 2)
----- End Fatal Exception
-------------------------------------------------
* die 'Failure using recoMET_caloMet_cfg.py' 84
* echo Failure using recoMET_caloMet_cfg.py: status 84
Failure using recoMET_caloMet_cfg.py: status 84
* exit 84
status = 21504
---> test testRecoMETMETProducers had ERRORS
^^^^ End Test testRecoMETMETProducers ^^^^
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18127 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbn_cZ6bKqzbIn0-b24BtMpbkRMvCks5rs5DMgaJpZM4MtpeH>.
|
please test |
The tests are being triggered in jenkins. |
personally I think a test with so many spurious failures should be restructured or deleted... |
Comparison job queued. |
+1 |
Three changes here:
Add the segment DPhi to the ME0Segments
This will be used in global reconstruction and in the trigger...it is better to calculate it once correctly. Incidentally, the ME0Segment size should be smaller now since we were unnecessarily keeping other variables with double precision. I added the computer as a static function that takes Points/Vectors as inputs. This is for matching to pixel tracks to segments, so that one can use the projected track's properties and get the proper Dphi.
Add central BX association
We have required that the ME0Segment is associated to the central BX in all studies, but did not apply it on the ME0Segment producing stage. However, these segments will be used a few times in the global muon reconstruction, so we thought it is best to filter out the bad segments here.
Default segment parameters
The parameters have been updated to match the baseline TDR granularity: 8 eta partitions/384 strips
++ one bug fix:
A change in the ME0Chamber geometry code:
The layer() functions were not working correctly...they iterated over layers but checked eta partition numbers.