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
Modernized AlcaBeamMonitor #31354
Modernized AlcaBeamMonitor #31354
Changes from all commits
7135c03
2143166
f87ebe7
d74ce46
706cb39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
<bin file="testAlcaBeamMonitor.cc"> | ||
<use name="FWCore/TestProcessor"/> | ||
<use name="catch2"/> | ||
</bin> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
#include "FWCore/TestProcessor/interface/TestProcessor.h" | ||
|
||
#define CATCH_CONFIG_MAIN | ||
#include "catch.hpp" | ||
|
||
TEST_CASE("AlcaBeamMonitor tests", "[AlcaBeamMonitor]") { | ||
//The python configuration | ||
edm::test::TestProcessor::Config config{ | ||
R"_(from FWCore.TestProcessor.TestProcess import * | ||
from DQM.BeamMonitor.AlcaBeamMonitor_cfi import AlcaBeamMonitor | ||
process = TestProcess() | ||
process.beamMonitor = AlcaBeamMonitor | ||
process.moduleToTest(process.beamMonitor) | ||
process.add_(cms.Service("DQMStore")) | ||
)_"}; | ||
|
||
SECTION("Run with no Lumis") { | ||
edm::test::TestProcessor tester{config}; | ||
tester.testRunWithNoLuminosityBlocks(); | ||
//get here without an exception or crashing | ||
REQUIRE(true); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -221,6 +221,43 @@ BeamFitter::~BeamFitter() { | |||||
delete MyPVFitter; | ||||||
} | ||||||
|
||||||
void BeamFitter::fillDescription(edm::ParameterSetDescription &iDesc) { | ||||||
edm::ParameterSetDescription beamFitter; | ||||||
|
||||||
beamFitter.addUntracked<bool>("Debug"); | ||||||
beamFitter.addUntracked<edm::InputTag>("TrackCollection"); | ||||||
iDesc.addUntracked<edm::InputTag>("primaryVertex", edm::InputTag("offlinePrimaryVertices")); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the default values for these parameters are defined here, would it make sense to remove them from the constructor:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the main python definition of BeamFitter is here [1] (with some alternate defitions in neighboring files) - do we want to reduce the duplication of the default parameters somewhat between the fillDescriptions and the cff? [1]
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’d prefer to have that done in a separate pull request so we can get the bug fix in. |
||||||
iDesc.addUntracked<edm::InputTag>("beamSpot", edm::InputTag("offlineBeamSpot")); | ||||||
beamFitter.addUntracked<bool>("WriteAscii"); | ||||||
beamFitter.addUntracked<std::string>("AsciiFileName"); | ||||||
beamFitter.addUntracked<bool>("AppendRunToFileName"); | ||||||
beamFitter.addUntracked<bool>("WriteDIPAscii"); | ||||||
// Specify whether we want to write the DIP file even if the fit is failed. | ||||||
beamFitter.addUntracked<bool>("WriteDIPOnBadFit", true); | ||||||
beamFitter.addUntracked<std::string>("DIPFileName"); | ||||||
beamFitter.addUntracked<bool>("SaveNtuple"); | ||||||
beamFitter.addUntracked<bool>("SaveFitResults"); | ||||||
beamFitter.addUntracked<bool>("SavePVVertices"); | ||||||
beamFitter.addUntracked<bool>("IsMuonCollection"); | ||||||
|
||||||
beamFitter.addUntracked<double>("MinimumPt"); | ||||||
beamFitter.addUntracked<double>("MaximumEta"); | ||||||
beamFitter.addUntracked<double>("MaximumImpactParameter"); | ||||||
beamFitter.addUntracked<double>("MaximumZ"); | ||||||
beamFitter.addUntracked<int>("MinimumTotalLayers"); | ||||||
beamFitter.addUntracked<int>("MinimumPixelLayers"); | ||||||
beamFitter.addUntracked<double>("MaximumNormChi2"); | ||||||
beamFitter.addUntracked<std::vector<std::string> >("TrackAlgorithm"); | ||||||
beamFitter.addUntracked<std::vector<std::string> >("TrackQuality"); | ||||||
beamFitter.addUntracked<int>("MinimumInputTracks"); | ||||||
beamFitter.addUntracked<double>("FractionOfFittedTrks"); | ||||||
beamFitter.addUntracked<double>("InputBeamWidth", -1.); | ||||||
|
||||||
beamFitter.addUntracked<std::string>("OutputFileName", ""); | ||||||
|
||||||
iDesc.add<edm::ParameterSetDescription>("BeamFitter", beamFitter); | ||||||
} | ||||||
|
||||||
void BeamFitter::readEvent(const edm::Event &iEvent) { | ||||||
frun = iEvent.id().run(); | ||||||
const edm::TimeValue_t ftimestamp = iEvent.time().value(); | ||||||
|
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.
Could you explain why you have removed the second parameter
"YourSubsystemName"
here but not inAlcaBeamMonitor::fillDescriptions
?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.
Having it in both the
getUntrackedParameter
call and infillDescriptions
would be redundant. If the parameter is missing, thefillDescriptions
call will inject the"MonitorName"
string parameter into the module's PSet using the default"YourSubsystemName"
. Therefore during the constructor call the parameter"MonitorName"
is guaranteed to exist so no default value is needed for thegetUntrackedParameter
. So the change I made has exactly the same ultimate behavior as the original code.In general, the framework group highly encourages everyone to stop specifying default values in
getUntrackedParameter
and instead usefillDescriptions
.