Implement an EDFilter to select events based on their bunch crossing number (10.3.x) #25019
Conversation
|
backport #25018 |
|
A new Pull Request was created by @fwyzard (Andrea Bocci) for CMSSW_10_3_X. It involves the following packages: FWCore/Modules @cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
type new-feature |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
tests-started
tests-pending
and removed
tests-pending
tests-started
labels
|
Pull request #25019 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again. |
2 similar comments
|
Pull request #25019 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again. |
|
Pull request #25019 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again. |
Implement a global EDFilter "BunchCrossingFilter" to select events based on their bunch cfrossing number (between 1 and 3564 inclusive). Include a test configuration based on 2018 data.
fwyzard
force-pushed
the
BunchCrossingFilter_103x
branch
from
63e9663 to
618cd1f
Compare
|
Pull request #25019 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again. |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
fwyzard
changed the title
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
|
||
| # input data | ||
| process.source = cms.Source('PoolSource', | ||
| fileNames = cms.untracked.vstring('/store/data/Run2018B/ZeroBias/RAW/v1/000/317/435/00000/10FA8B3F-5A68-E811-A1D9-FA163E481185.root') |
There was a problem hiding this comment.
I want to avoid getting files from storage for framework tests since I do run these test on my laptop without internet connections. We normally either create new input files for the test or we store tiny ROOT files into the repository. Would any of the files in IOPool/Input/testdata work for your test?
There was a problem hiding this comment.
I think the files under IOPool/Input/testdata have at most one event, so it is hard to show the various ways how to use the filter.
Of course if this is a requirement, I can simplify the test case and adapt it to that.
There was a problem hiding this comment.
You could stick a tiny file in IOPool/Input/testdata if you want, or one in FWCore/Modules/testdata
There was a problem hiding this comment.
How do I create a tiny file ?
Even if I drop all collections ("drop *"), a file with 1 event is ~1.3 MB.
There was a problem hiding this comment.
I tried the following
import FWCore.ParameterSet.Config as cms
process = cms.Process("SLIM")
process.source = cms.Source("PoolSource", fileNames = cms.untracked.vstring("file:10FA8B3F-5A68-E811-A1D9-FA163E481185.root"))
process.out = cms.OutputModule("PoolOutputModule", fileName = cms.untracked.string("slim.root"),
dropMetaData = cms.untracked.string("ALL"),
outputCommands = cms.untracked.vstring("drop *")
)
process.maxEvents = cms.untracked.PSet(input = cms.untracked.int32(10))
process.o = cms.EndPath(process.out)
the generated file was 1.5MB in size. Looking at the internals, the ParameterSets branch is 1.47MB in size. I'll see if I can come up with a simple way to fake that.
There was a problem hiding this comment.
If you checkout IOPool/Provenance and edit IOPool/Provenance/src/CommonProvenanceFiller.cc and comment out the loop
and rebuild IOPool/Output. At that point, using the configuration I tested, you can create a slim.root file which is only 40KB in length. I was successfully able to read back that tiny file from a simple cmsRun job.
|
An alternative testing strategy could be to use However, it would require some new ability in that code to manipulate the EventPrincipal. |
|
The change in the TestProcessor would be right here since the |
|
@fwyzard how do you want to proceed with the test you added? The more I think about it, the better I like the TestProcessor approach since it avoids the neef for an auxilliary file and gives more control over the testing. |
|
Hi @Dr15Jones, Let me know, |
|
@fwyzard It seems to me like there are actually two tests being proposed
The Even with that in mind, the configuration you have made doesn't seem to have a testable failure condition and does not have a launching script so it would not help in catching problems. |
|
@fwyzard was the configuration you wrote maybe intended to be an example of use? If so, then instead of a configuration in the test directory you could
|
|
@Dr15Jones, if I understand correctly, the tests you are proposing would serve to test the underlying framework features related to the handling of the bunch crossing number in data (and MC). Of course I have nothing against that, but that goes beyond the scope of adding a test that shows how the new filter is supposed to work.
I appreciate that both solutions would still include the documentation in the code base. However, I do not know who would go looking for it there (in the doxygen docs or in a .md file). If having a meaningful and realistic example in the test directory is the only showstopper, I can move the filter under a different package (e.g. |
|
The showstopper is requiring reading a file in a test that is not part of the repository. That would cause problems for testing the framework stand alone (which is something I do regularly from my laptop). There was the alternative of generating a 'tiny' file, putting it in IOPool/Input/testdata and having your test configuration read that one. |
|
I'll take @fabiocos suggestion on how to proceed. |
|
@fwyzard @Dr15Jones sorry, which is finally the size of this file? 1.5 MB or 40 kB? In general we are trying to push whatever non trivial data file in external packages, but checking the present status of the release I see: 13:49 cmsdev15 2948> find -L /cvmfs/cms-ib.cern.ch/week0/slc6_amd64_gcc700/cms/cmssw/CMSSW_10_4_X_2018-12-13-2300/src/IOPool/ -name "*.root" -exec du -k {} ; | sort -n | less Another 40 kB file is not something unmanageable, even if I wonder whether we should not put all these files into an external data package (still part of the code distribution, but in a different package/git repository). In that case we could even add 1.5 MB. |
|
The original approach proposed by Chris to create a slim input file should be OK, I only have to go through recalling the input file and following the recipe. |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #25019 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again. |
|
It turned out I need a slightly bigger file (~120kB) to exercise all the various cases. |
|
By the way, we can probably close this specific PR, unless this functionality is deemed useful for 10.3.x. |
|
@fwyzard fine for me to close this PR and follow up with the others |
|
Moved to 10.2.x and 10.4.x only. |
Implement a global EDFilter
BunchCrossingFilterto select events based on their bunch crossing number (between 1 and 3564 inclusive).Include a test configuration based on 2018 data.