Skip to content

Updating fillDescriptions in RecoBTag packages #49274

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Updating fillDescriptions in RecoBTag packages #49274

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5946ac2
Add files via upload
soureek Oct 31, 2025
88e7346
Add files via upload
soureek Oct 31, 2025
3a846f4
Add files via upload
soureek Oct 31, 2025
43d803c
Add files via upload
soureek Oct 31, 2025
09c6fba
Add files via upload
soureek Oct 31, 2025
cd6286c
Add files via upload
soureek Oct 31, 2025
830e13c
Fixing code styles with 'scram build code-format' command
soureek Oct 31, 2025
9c990cf
Updating to untracked parameters
soureek Oct 31, 2025
f9778bd
Resolving code-format issues for RecoBTag/Skimming/src/BTagSkimLepton…
soureek Oct 31, 2025
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
1 change: 1 addition & 0 deletions RecoBTag/BTagTools/BuildFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<use name="TrackingTools/GeomPropagators"/>
<use name="TrackingTools/PatternTools"/>
<use name="TrackingTools/TrajectoryState"/>
<use name="FWCore/ParameterSet"/>
<use name="clhep"/>
<export>
<lib name="1"/>
Expand Down
3 changes: 3 additions & 0 deletions RecoBTag/BTagTools/interface/SignedDecayLength3D.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include "TrackingTools/TransientTrack/interface/TransientTrack.h"
#include "DataFormats/VertexReco/interface/Vertex.h"
#include "DataFormats/GeometryVector/interface/GlobalVector.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
#include <utility>

/** Threedimensional track decay length (minimum distance of the closest
Expand All @@ -23,6 +25,7 @@ class SignedDecayLength3D {
const reco::Vertex &vertex);

int id() const { return 3; }
static void fillDescriptions(edm::ConfigurationDescriptions &descriptions);

private:
static TrajectoryStateOnSurface closestApproachToJet(const FreeTrajectoryState &,
Expand Down
4 changes: 4 additions & 0 deletions RecoBTag/BTagTools/interface/SignedImpactParameter3D.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include "TrackingTools/TransientTrack/interface/TransientTrack.h"
#include "DataFormats/VertexReco/interface/Vertex.h"
#include "DataFormats/GeometryVector/interface/GlobalVector.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
#include <utility>
/** Threedimensional track impact parameter signed according to the jet
* direction
Expand All @@ -31,6 +33,8 @@ class SignedImpactParameter3D {
const GlobalVector &direction,
const reco::Vertex &vertex);

static void fillDescriptions(edm::ConfigurationDescriptions &descriptions);

private:
static GlobalVector distance(const TrajectoryStateOnSurface &, const reco::Vertex &, const GlobalVector &);

Expand Down
5 changes: 5 additions & 0 deletions RecoBTag/BTagTools/interface/SignedTransverseImpactParameter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
#include "TrackingTools/TransientTrack/interface/TransientTrack.h"
#include "DataFormats/VertexReco/interface/Vertex.h"
#include "DataFormats/GeometryVector/interface/GlobalVector.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

#include <utility>

/** Transverse track impact parameter signed according to the jet
Expand All @@ -23,6 +26,8 @@ class SignedTransverseImpactParameter {
std::pair<bool, Measurement1D> zImpactParameter(const reco::TransientTrack &,
const GlobalVector &,
const reco::Vertex &) const;

static void fillDescriptions(edm::ConfigurationDescriptions &descriptions);
};

#endif
6 changes: 6 additions & 0 deletions RecoBTag/BTagTools/src/SignedDecayLength3D.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

using namespace std;
using namespace reco;
using namespace edm;

pair<bool, Measurement1D> SignedDecayLength3D::apply(const TransientTrack& transientTrack,
const GlobalVector& direction,
Expand Down Expand Up @@ -76,3 +77,8 @@ TrajectoryStateOnSurface SignedDecayLength3D::closestApproachToJet(const FreeTra

return TETL.extrapolate(aFTS, Jet);
}

void SignedDecayLength3D::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
descriptions.addWithDefaultLabel(desc);
}
6 changes: 6 additions & 0 deletions RecoBTag/BTagTools/src/SignedImpactParameter3D.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

using namespace std;
using namespace reco;
using namespace edm;

pair<bool, Measurement1D> SignedImpactParameter3D::apply(const TransientTrack& transientTrack,
const GlobalVector& direction,
Expand Down Expand Up @@ -189,3 +190,8 @@ pair<double, Measurement1D> SignedImpactParameter3D::distanceWithJetAxis(const T

return pair<double, Measurement1D>(theDistanceAlongJetAxis, DTJA);
}

void SignedImpactParameter3D::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
descriptions.addWithDefaultLabel(desc);
}
6 changes: 6 additions & 0 deletions RecoBTag/BTagTools/src/SignedTransverseImpactParameter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using namespace std;
using namespace reco;
using namespace edm;

pair<bool, Measurement1D> SignedTransverseImpactParameter::apply(const TransientTrack& track,
const GlobalVector& direction,
Expand Down Expand Up @@ -117,3 +118,8 @@ pair<bool, Measurement1D> SignedTransverseImpactParameter::zImpactParameter(cons

return pair<bool, Measurement1D>(true, Measurement1D(deltaZ, errZ));
}

void SignedTransverseImpactParameter::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
descriptions.addWithDefaultLabel(desc);
}
4 changes: 4 additions & 0 deletions RecoBTag/Skimming/interface/BTagSkimLeptonJet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Utilities/interface/InputTag.h"

#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

class BTagSkimLeptonJet : public edm::one::EDFilter<> {
public:
explicit BTagSkimLeptonJet(const edm::ParameterSet&);
~BTagSkimLeptonJet() override;
bool filter(edm::Event&, const edm::EventSetup&) override;
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
void endJob() override;

private:
Expand Down
3 changes: 3 additions & 0 deletions RecoBTag/Skimming/interface/BTagSkimMC.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "FWCore/Framework/interface/stream/EDFilter.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

namespace BTagSkimMCCount {
struct Counters {
Expand All @@ -20,6 +22,7 @@ class BTagSkimMC : public edm::stream::EDFilter<edm::GlobalCache<BTagSkimMCCount
bool filter(edm::Event& evt, const edm::EventSetup& es) override;
void endStream() override;
static void globalEndJob(const BTagSkimMCCount::Counters* counters);
static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);

private:
bool verbose;
Expand Down
4 changes: 4 additions & 0 deletions RecoBTag/Skimming/src/BTagSkimLeptonJet.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ BTagSkimLeptonJet::~BTagSkimLeptonJet() {}

/*------------------------------------------------------------------------*/

void BTagSkimLeptonJet::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
descriptions.addWithDefaultLabel(desc);
}
bool BTagSkimLeptonJet::filter(edm::Event& iEvent, const edm::EventSetup& iSetup) {
nEvents_++;

Expand Down
6 changes: 5 additions & 1 deletion RecoBTag/Skimming/src/BTagSkimMC.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ void BTagSkimMC::globalEndJob(const BTagSkimMCCount::Counters* count) {
<< std::endl;
}

#include "FWCore/Framework/interface/MakerMacros.h"
void BTagSkimMC::fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
Copy link
Contributor

@mmusich mmusich Oct 31, 2025

Choose a reason for hiding this comment

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

Sorry, but what's the point of this?
This does not provide any validation nor defaults for any of the actual configuration parameters?

Copy link
Author

@soureek soureek Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@mmusich These are primarily helper classes or EDFilters. So they do not require anything specific validation or defaults.
All the EDProducers, EDAnalyzers, ESProducers under RecoBTag have the necessary detailed fillDescriptions provided already.

Copy link
Contributor

@mmusich mmusich Oct 31, 2025

Choose a reason for hiding this comment

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

@soureek

cmssw/RecoBTag/Skimming/src/BTagSkimMC.cc

Lines 14 to 17 in 830e13c

verbose = p.getUntrackedParameter<bool>("verbose", false);
pthatMin = p.getParameter<double>("pthat_min");
pthatMax = p.getParameter<double>("pthat_max");
process_ = p.getParameter<string>("mcProcess");

This specific class has configurable parameters. I am sure others do elsewhere

Copy link
Author

@soureek soureek Oct 31, 2025

Choose a reason for hiding this comment

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

@mmusich I've changed most of them to UntrackedParameter except for the InputTags so that the default values are set in the constructor itself. The relevant python cfi files are modified accordingly.

Copy link
Contributor

@mmusich mmusich Oct 31, 2025

Choose a reason for hiding this comment

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

This is exactly the opposite of what should have been done.

Copy link
Author

@soureek soureek Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In my limited experience, I don't remember encountering issues regarding time or memory consumption with Untracked parameters.
Can you please elaborate may be with some reference @mmusich ?

Copy link
Contributor

@mmusich mmusich Nov 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@soureek the problem is not about time or memory performance.
It's jut that I don't see the what's the advantage of the approach you propose:

  1. the tracked-ness of a given parameter is what makes it visible in the event provenance. Generally parameters that influence the physics performance are tracked, while those that don't are not. Have you checked if all the parameters changed here have influence on the reconstruction performance?
  2. making them untracked still doesn't not solve the problem that when providing a fillDescriptions method, one is supposed to support configuration validation (and defaults) for all the configurable parameters (see cmssw code rule 6.14 at https://cms-sw.github.io/cms_coding_rules.html#6--packaging-rules-1)

These are primarily helper classes or EDFilters. So they do not require anything specific validation or defaults.
All the EDProducers, EDAnalyzers, ESProducers under RecoBTag have the necessary detailed fillDescriptions provided already.

that is indeed the case, because previously these have been adapted as part of a bigger issue to support them (see #47275 and pull requests mentioned therein).

IMHO the right course of action is to create fillDescriptions for the ancillary helper classes (providing validation and defaults for their configurable parameters) and then use those methods to populate in turn the fillDescriptions of the plugins that used them (suggested reading: https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Plugin_Module_Using_Another_He).

Copy link
Author

@soureek soureek Nov 2, 2025

Choose a reason for hiding this comment

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

@mmusich Thanks for the detailed explanation. I have a better understanding of the issue now. I will make a fresh PR based on the ☝️ comments. You can close this one without merging if that's suitable.

descriptions.addWithDefaultLabel(desc);
}

#include "FWCore/Framework/interface/MakerMacros.h"
DEFINE_FWK_MODULE(BTagSkimMC);