Skip to content

PPS: association cut fix #35941

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.

Sign up for GitHub

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

Merged
cmsbuild merged 6 commits into from
Nov 3, 2021
Merged

PPS: association cut fix #35941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9512cc1
replace std::vector::at with operator[]
jan-kaspar Nov 1, 2021
8362ee6
remove mutable class members, use REGISTER_PLUGIN_INIT instead
jan-kaspar Nov 1, 2021
3849f99
code format
jan-kaspar Nov 1, 2021
beaf37b
replace std::map::at with find
jan-kaspar Nov 2, 2021
3bd6d0b
validate structure during initialisation
jan-kaspar Nov 2, 2021
4eb4170
code format
jan-kaspar Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion CondCore/CTPPSPlugins/src/plugin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
#include "CondFormats/PPSObjects/interface/PPSAssociationCuts.h"
#include "CondFormats/DataRecord/interface/PPSAssociationCutsRcd.h"

namespace {
struct InitAssociationCuts {
void operator()(PPSAssociationCuts &cuts) { cuts.initialize(); }
};
} // namespace

REGISTER_PLUGIN(CTPPSBeamParametersRcd, CTPPSBeamParameters);
REGISTER_PLUGIN(CTPPSPixelDAQMappingRcd, CTPPSPixelDAQMapping);
REGISTER_PLUGIN(CTPPSPixelAnalysisMaskRcd, CTPPSPixelAnalysisMask);
Expand All @@ -39,4 +45,5 @@ REGISTER_PLUGIN(PPSDirectSimulationDataRcd, PPSDirectSimulationData);
REGISTER_PLUGIN(PPSPixelTopologyRcd, PPSPixelTopology);
REGISTER_PLUGIN(PPSAlignmentConfigRcd, PPSAlignmentConfig);
REGISTER_PLUGIN(PPSAlignmentConfigurationRcd, PPSAlignmentConfiguration);
REGISTER_PLUGIN(PPSAssociationCutsRcd, PPSAssociationCuts);

REGISTER_PLUGIN_INIT(PPSAssociationCutsRcd, PPSAssociationCuts, InitAssociationCuts);
16 changes: 11 additions & 5 deletions CondFormats/PPSObjects/interface/PPSAssociationCuts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ struct TF1;
#include "CondFormats/Serialization/interface/Serializable.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
#include "FWCore/Utilities/interface/Exception.h"

#include <iostream>
#include <memory>
Expand All @@ -37,8 +38,7 @@ class PPSAssociationCuts {
double getTiTrMax() const { return ti_tr_max_; }

// build TF1 representations of the mean and threshold functions
// NB: declared as const as it only modifies mutable class fields
void buildFunctions() const;
void buildFunctions();

// returns whether the specified cut is applied
bool isApplied(Quantities quantity) const;
Expand All @@ -52,8 +52,8 @@ class PPSAssociationCuts {
std::vector<std::string> s_thresholds_;

// TF1 representation of the cut parameters - for run time evaluations
mutable std::vector<std::shared_ptr<TF1> > f_means_ COND_TRANSIENT;
mutable std::vector<std::shared_ptr<TF1> > f_thresholds_ COND_TRANSIENT;
std::vector<std::shared_ptr<TF1> > f_means_ COND_TRANSIENT;
std::vector<std::shared_ptr<TF1> > f_thresholds_ COND_TRANSIENT;

// timing-tracking cuts
double ti_tr_min_;
Expand All @@ -70,7 +70,13 @@ class PPSAssociationCuts {

~PPSAssociationCuts() {}

const CutsPerArm &getAssociationCuts(const int sector) const { return association_cuts_.at(sector); }
// checks if the data have a valid structure
bool isValid() const;

// builds run-time data members, useful e.g. after loading data from DB
void initialize();

const CutsPerArm &getAssociationCuts(const int sector) const { return association_cuts_.find(sector)->second; }
Copy link
Contributor

@makortel makortel Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to point out that with this approach a incorrect payload or sector value lead to undefined behavior (crashes at best, silent garbage output at worst).

Copy link
Contributor Author

@jan-kaspar jan-kaspar Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am aware. I wonder what the best/recommended solution would be... Possibly we could update the initialize method to validate the content. But what to do if wrong? Throw an exception? Is this allowed/recommended?

Copy link
Contributor

@VinInn VinInn Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing a check and throw (ie abort) at inizialize time is ok.
What is NOT ok is to check validity at each single access

Copy link
Contributor Author

@jan-kaspar jan-kaspar Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @VinInn! Then I will push one more commit in an hour or so.


static edm::ParameterSetDescription getDefaultParameters();

Expand Down
42 changes: 31 additions & 11 deletions CondFormats/PPSObjects/src/PPSAssociationCuts.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,11 @@ PPSAssociationCuts::CutsPerArm::CutsPerArm(const edm::ParameterSet &iConfig, int

ti_tr_min_ = association_cuts.getParameter<double>("ti_tr_min");
ti_tr_max_ = association_cuts.getParameter<double>("ti_tr_max");

buildFunctions();
}

//----------------------------------------------------------------------------------------------------

void PPSAssociationCuts::CutsPerArm::buildFunctions() const {
void PPSAssociationCuts::CutsPerArm::buildFunctions() {
f_means_.clear();
for (const auto &s : s_means_)
f_means_.push_back(std::make_shared<TF1>("f", s.c_str()));
Expand All @@ -46,7 +44,7 @@ void PPSAssociationCuts::CutsPerArm::buildFunctions() const {
//----------------------------------------------------------------------------------------------------

bool PPSAssociationCuts::CutsPerArm::isApplied(Quantities quantity) const {
return (!s_thresholds_.at(quantity).empty()) && (!s_means_.at(quantity).empty());
return (!s_thresholds_[quantity].empty()) && (!s_means_[quantity].empty());
}

//----------------------------------------------------------------------------------------------------
Expand All @@ -57,14 +55,9 @@ bool PPSAssociationCuts::CutsPerArm::isSatisfied(
if (!isApplied(quantity))
return true;

// build functions if not already done
// (this may happen if data (string representation) are loaded from DB and the constructor is not executed)
if (f_means_.size() < s_means_.size())
buildFunctions();

// evaluate mean and threshold
const double mean = evaluateExpression(f_means_.at(quantity), x_near, y_near, xangle);
const double threshold = evaluateExpression(f_thresholds_.at(quantity), x_near, y_near, xangle);
const double mean = evaluateExpression(f_means_[quantity], x_near, y_near, xangle);
const double threshold = evaluateExpression(f_thresholds_[quantity], x_near, y_near, xangle);

// make comparison
return fabs(q_NF_diff - mean) < threshold;
Expand All @@ -88,6 +81,8 @@ PPSAssociationCuts::PPSAssociationCuts(const edm::ParameterSet &iConfig) {
unsigned int i = 0;
for (const int &sector : {45, 56})
association_cuts_[i++] = CutsPerArm(iConfig, sector);

initialize();
}

//----------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -115,6 +110,31 @@ edm::ParameterSetDescription PPSAssociationCuts::getDefaultParameters() {

//----------------------------------------------------------------------------------------------------

bool PPSAssociationCuts::isValid() const {
// valid if association_cuts_ have two entries, with keys 0 and 1
if (association_cuts_.size() != 2)
return false;

if (association_cuts_.find(0) == association_cuts_.end())
return false;
if (association_cuts_.find(1) == association_cuts_.end())
return false;

return true;
}

//----------------------------------------------------------------------------------------------------

void PPSAssociationCuts::initialize() {
if (!isValid())
throw cms::Exception("PPS") << "Invalid structure of PPSAssociationCuts.";

for (auto &p : association_cuts_)
p.second.buildFunctions();
}

//----------------------------------------------------------------------------------------------------

std::ostream &operator<<(std::ostream &os, const PPSAssociationCuts::CutsPerArm &cutsPerArm) {
os << "CutsPerArm {" << std::endl;

Expand Down