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

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

Updating fillDescriptions in RecoBTag packages #49274

wants to merge 9 commits into from

Conversation

@soureek
Copy link

@soureek soureek commented Oct 31, 2025

PR description:

Updating fillDescriptions for RecoBTag/BTagTools and RecoBTag/Skimming following issue #29265

Local compilation successful without any warnings.

@cmsbuild cmsbuild added this to the CMSSW_16_0_X milestone Oct 31, 2025
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025 •
edited
Loading

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49274/46632

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49274/46633

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025

A new Pull Request was created by @soureek for master.

It involves the following packages:

  • RecoBTag/BTagTools (reconstruction)
  • RecoBTag/Skimming (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks.
@AlexDeMoor, @Ming-Yan, @Senphy, @andrzejnovak, @castaned, @pavlo-kashko, @philippgadow this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

mmusich
mmusich reviewed Oct 31, 2025
View reviewed changes
RecoBTag/Skimming/src/BTagSkimMC.cc

#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.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49274/46636

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49274/46637

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 31, 2025

Pull request #49274 was updated. @cmsbuild, @jfernan2, @mandrenguyen, @srimanob can you please check and sign again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mmusich mmusich mmusich left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

code-checks-approved orp-pending pending-signatures reconstruction-pending tests-pending

Projects

None yet

Milestone

CMSSW_16_0_X

Development

Successfully merging this pull request may close these issues.

3 participants

@soureek @cmsbuild @mmusich