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 3 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);
13 changes: 9 additions & 4 deletions CondFormats/PPSObjects/interface/PPSAssociationCuts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,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 +51,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,6 +69,12 @@ class PPSAssociationCuts {

~PPSAssociationCuts() {}

// builds run-time data members, useful e.g. after loading data from DB
void initialize() {
for (auto &p : association_cuts_)
p.second.buildFunctions();
}

const CutsPerArm &getAssociationCuts(const int sector) const { return association_cuts_.at(sector); }
Copy link
Contributor

@tvami tvami Nov 2, 2021

Choose a reason for hiding this comment

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

this is the line @malbouis meant

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! I see now. At the moment of opening the PR, there was only this answer: #35936 (comment), which I interpret that at is OK for a map. Later, another answer was provided: #35936 (comment), which seems to be in disagreement with the first one. I guess I need a decision which hint to follow. If there is no clear guideline, keeping the code as is saves my time.

Copy link
Contributor

@malbouis malbouis Nov 2, 2021

Choose a reason for hiding this comment

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

@makortel @VinInn , is it ok to keep the .at() in this case?

Copy link
Contributor

@tvami tvami Nov 2, 2021

Choose a reason for hiding this comment

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

Hi @jan-kaspar my interpretation is that Matti is saying at is OK if we are OK with throwing an exception, while Vincenzo is saying it is not OK to throw the exception, thus at is also not OK. I think find should be used.

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.

The real question is: does one really need a map?
Is not a trivial array enough? (what values of "arm" should be supported?)

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.

What would be the circumstances if an element is not found? I'm wondering mostly between sector comes from data (directly or via some processing) and can go out of bounds occasionally, and sector originally comes from configuration and out of bounds values would be logic errors.

What would the calling code do if an element is not found?

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.

The sector refers to the LHC sectors around IP5: these are two and will never change. I agree that array/vector would have been a better design. The code has been made by a student and I didn't catch this - sorry for that. Now that the class is already used in CondDB, I don't want to change it anymore. On the other hand, doing the search (by map::find) on a map with two elements is not that costly, so I believe we are having a sort of academic discussion. I will commit the change to map::find soon.

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.

For the moment there is no guarantee in the code that the DB payload will contain data for both LHC sectors. We assume that the person who uploads the conditions to DB does this well. Let me know if you wish me to put some "protection" to the code.

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.

The sector refers to the LHC sectors around IP5: these are two and will never change. I agree that array/vector would have been a better design. The code has been made by a student and I didn't catch this - sorry for that. Now that the class is already used in CondDB, I don't want to change it anymore. On the other hand, doing the search (by map::find) on a map with two elements is not that costly, so I believe we are having a sort of academic discussion. I will commit the change to map::find soon.

Done in beaf37b.


static edm::ParameterSetDescription getDefaultParameters();
Expand Down
13 changes: 4 additions & 9 deletions CondFormats/PPSObjects/src/PPSAssociationCuts.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ PPSAssociationCuts::CutsPerArm::CutsPerArm(const edm::ParameterSet &iConfig, int

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

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 +46,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 +57,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 Down