Skip to content
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

Backport of LHEGenericMassFilter to 106X #36754

Merged
merged 4 commits into from
Jan 30, 2022

Conversation

JanFSchulte
Copy link
Contributor

This PR backports the LHE filter designed to cut on the invariant mass of a number of particles introduced in #35938. Backport to 10_6_X is necessary since some high dilepton mass ttbar and WW samples need to be produced in the UL campaigns.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @JanFSchulte (Jan-Frederik Schulte) for CMSSW_10_6_X.

It involves the following packages:

  • GeneratorInterface/GenFilters (generators)

@SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please review it and eventually sign? Thanks.
@alberto-sanchez, @mkirsano this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

Pull request #36754 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @agrohsje, @alberto-sanchez can you please check and sign again.

@agrohsje
Copy link

Hi @qliphy @perrotta , I removed myself from the l2-generators team but I still get mails about PR's. Does it take a while to update or should I remove my name somewhere else?

@agrohsje
Copy link

please test

@perrotta
Copy link
Contributor

@agrohsje I still see your nick listed for "generators" in https://github.com/cms-sw/cms-bot/blob/master/categories.py

The following bot PR removes it:
cms-sw/cms-bot#1692
If it is actually what you wan't, please confirm and we'll merge it.

@JanFSchulte
Copy link
Contributor Author

I am not sure what the issue is here. In the tests I see only workflow 4.76 failing, which I can not check locally since the input files are not found at T2_CH_CERN. The code introduced here is not used in any of the tests, so I can't see how a failure would be related.

@perrotta
Copy link
Contributor

I am not sure what the issue is here. In the tests I see only workflow 4.76 failing, which I can not check locally since the input files are not found at T2_CH_CERN. The code introduced here is not used in any of the tests, so I can't see how a failure would be related.

@JanFSchulte the crash in wf 4.76 is unrelated (see #36771). Please disregard that workflow for now

@SiewYan
Copy link
Contributor

SiewYan commented Jan 24, 2022

@perrotta , should i re-trigger the test and avoid 4.76 to complete the test?

@JanFSchulte
Copy link
Contributor Author

Could someone comment on the status of this PR? We would like to have if merged so we know we can move ahead with preparing the sample requests.

class LHEGenericMassFilter : public edm::global::EDFilter<> {
public:
explicit LHEGenericMassFilter(const edm::ParameterSet&);
~LHEGenericMassFilter() override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
~LHEGenericMassFilter() override;
~LHEGenericMassFilter() override = default;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 13 to 16
LHEGenericMassFilter::~LHEGenericMassFilter() {
// do anything here that needs to be done at destruction time
// (e.g. close files, deallocate resources etc.)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
LHEGenericMassFilter::~LHEGenericMassFilter() {
// do anything here that needs to be done at destruction time
// (e.g. close files, deallocate resources etc.)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


private:
bool filter(edm::StreamID, edm::Event&, edm::EventSetup const&) const override;
void endJob() override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Comment on lines 50 to 51
double Mass = std::sqrt(E * E - (Px * Px + Py * Py + Pz * Pz));
if (Mass > minMass_ && Mass < maxMass_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
double Mass = std::sqrt(E * E - (Px * Px + Py * Py + Pz * Pz));
if (Mass > minMass_ && Mass < maxMass_) {
double sqrdMass = (E * E - (Px * Px + Py * Py + Pz * Pz);
if (sqrdMass > minMass_*minMass_ && Mass < maxMass_*maxMass_) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@perrotta
Copy link
Contributor

As in the backported PR, I would move it into the \plugin area, also combining the header and implementation file into a single .cc
By the way, why didn't you backport it as it was?

@cmsbuild
Copy link
Contributor

Pull request #36754 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @alberto-sanchez can you please check and sign again.

@JanFSchulte
Copy link
Contributor Author

I have merged the files and moved them to plugins. I had split them up to conform to the structure of the other modules in this package, and because I had issues getting it to be included in the compilation when it was in the plugins folder. I realized now that was because there was no BuildFile.xml there, so now everything works.

@cmsbuild
Copy link
Contributor

Pull request #36754 was updated. @SiewYan, @mkirsano, @Saptaparna, @cmsbuild, @GurpreetSinghChahal, @alberto-sanchez can you please check and sign again.

@SiewYan
Copy link
Contributor

SiewYan commented Jan 28, 2022

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25e45e/22055/summary.html
COMMIT: cc76571
CMSSW: CMSSW_10_6_X_2022-01-26-1100/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36754/22055/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 3215686
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3215350
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 143 log files, 29 edm output root files, 35 DQM output files
  • TriggerResults: no differences found

@SiewYan
Copy link
Contributor

SiewYan commented Jan 30, 2022

+1

its look good to me.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_3_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Jan 30, 2022

+1

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

Successfully merging this pull request may close these issues.

None yet

6 participants