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

[EGM] Eta-Extended-Electrons (EEE) #35309

Merged
merged 10 commits into from Sep 30, 2021
Merged

Conversation

swagata87
Copy link
Contributor

PR description:

This PR aims to improve electron reconstruction efficiency in high eta region, 2.5<|eta|<3.0.

Motivation: EEEs can be important for many analyses, one example is measurement of weak mixing angle using forward-backward asymmetry of DY events.

Currently, electron reconstruction efficiency sharply falls off after around |eta|=2.5. This is because GSF tracking tends to fail in high eta, as the outer tracker coverage ends and we are left with only the inner tracker layers. Two parameters were identified, whose cut-values need to be relaxed, to improve GSF tracking efficiency in high eta region; they are the following:

(1) process.TrajectoryFilterForElectrons.minimumNumberOfHits = cms.int32(3)  ### default was 5
(2) process.GsfElectronFittingSmoother.MinNumberOfHits = cms.int32(3)  #### default was 5 

However, this change was made only for high eta electrons, keeping the parameters' cut values same in low eta.

Note that while we expect EEEs to enter 12_1 and MC samples to be produced with it, the PF Electron ID and energy regression for EEEs would likely be suboptimal. So a temporary switch is added to prevent EEEs to enter particle flow. When 12_1 samples will be available, we will retrain PF electron ID and energy regression including EEEs, and we will put in the updates in 12_2 while removing this switch and allowing EEEs to the particle flow. This is done as an intermediate, safe approach.

PR validation:

Some checks were made using 1000 double-electron gun events.
This plot shows electron eta distribution(from AOD); default CMSSW in blue and after merging this branch in green. More electrons are reconstructed in high eta, but there is no change in other regions.
Screen Shot 2021-09-16 at 19 19 56

This is general track eta distribution(from AOD), showing no change, as intended.
Screen Shot 2021-09-16 at 19 30 01

runTheMatrix.py -l 12434.0 ran fine (ttbar 2023)

This PR is not a backport.
Backport not needed.

This work is done together with Shilpi and Yash; in collaboration with PF group (Josh, Kenichi, Kenneth et. al) and with help from tracking experts (Mia et. al). Thanks to Marino(HLT-STORM) for helping with HLT customisation.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35309/25327

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35309/25328

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

  • HLTrigger/Configuration (hlt)
  • RecoParticleFlow/PFProducer (reconstruction)
  • TrackingTools/GsfTracking (reconstruction)
  • TrackingTools/TrackFitters (reconstruction)
  • TrackingTools/TrajectoryFiltering (reconstruction)

@Martin-Grunewald, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @abbiendi, @Martin-Grunewald, @mmusich, @cericeci, @mmarionncern, @JanFSchulte, @jhgoh, @lgray, @sscruz, @seemasharmafnal, @trocino, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @hatakeyamak, @ebrondol, @mtosi, @dgulhan, @HuguesBrun, @Fedespring, @calderona, @lecriste, @cbernet, @gpetruc 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

@slava77
Copy link
Contributor

slava77 commented Sep 16, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b20406/18675/summary.html
COMMIT: f8e66aa
CMSSW: CMSSW_12_1_X_2021-09-16-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35309/18675/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: 3494 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000833
  • DQMHistoTests: Total failures: 1069
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2999741
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 38 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@@ -17,6 +17,24 @@
# pset.minGoodStripCharge = cms.PSet(refToPSet_ = cms.string('HLTSiStripClusterChargeCutNone'))
# return process

# Eta Extended Electrons
def customiseForPRNUM(process):
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
def customiseForPRNUM(process):
def customiseFor35309(process):

Copy link
Contributor

Choose a reason for hiding this comment

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

Please change PRNUM to 35309, here and below (only a technicality, but necessary..).

@@ -134,5 +152,5 @@ def customizeHLTforCMSSW(process, menuType="GRun"):

# add call to action function in proper order: newest last!
# process = customiseFor12718(process)

process = customiseForPRNUM(process)
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
process = customiseForPRNUM(process)
process = customiseFor35309(process)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35309/25357

@cmsbuild
Copy link
Contributor

Pull request #35309 was updated. @Martin-Grunewald, @jpata, @cmsbuild, @slava77 can you please check and sign again.

if not hasattr(esp, 'MinNumberOfHitsHighEta'):
esp.MinNumberOfHitsHighEta = cms.int32(5)

return process
Copy link
Contributor

Choose a reason for hiding this comment

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

If these added parameters are all in their respective fillDescriptions method, then this customisation would not be needed. Is this the case or could it be achieved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem straightforward to me, as fillDescriptions function is not present in one of the codes where we added new parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which one? Could it be added there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this part can be deleted:

for esp in esproducers_by_type(process, 'KFFittingSmootherESProducer'):
        if not hasattr(esp, 'HighEtaSwitch'):
            esp.HighEtaSwitch = cms.double(5.0)
        if not hasattr(esp, 'MinNumberOfHitsHighEta'):
            esp.MinNumberOfHitsHighEta = cms.int32(5)

because TrackingTools/TrackFitters/plugins/KFFittingSmootherESProducer.cc already have fillDescriptions, and our new parameters are in there.

But the other part, the following, is needed:

    for pset in process._Process__psets.values():
        if hasattr(pset,'ComponentType'):
            if (pset.ComponentType == 'CkfBaseTrajectoryFilter'):
                if not hasattr(pset, 'highEtaSwitch'):
                    pset.highEtaSwitch = cms.double(5.0)
                if not hasattr(pset, 'minHitsAtHighEta'):
                    pset.minHitsAtHighEta = cms.int32(5)

because new parameters are added in TrackingTools/TrajectoryFiltering/interface/MinHitsTrajectoryFilter.h, and there is no fillDescriptions. Adding fillDescriptions in MinHitsTrajectoryFilter.cc doesn't solve the problem. It's not clear where we need to add fillDescriptions to make it work.

If possible, we would like to proceed with HLT customisation for now, and we will try to figure out fillDescriptions later on together with tracking experts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you then please remove the 'KFFittingSmootherESProducer' customisation as it is not needed?
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, HLTrigger/Configuration/python/customizeHLTforCMSSW.py shows a merge conflict, so it needs to be updated anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad on the customisation @swagata87; when we discussed it offline, I missed that it was not needed for one of the modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now removed KFFittingSmootherESProducer customisation and resolved the conflict...

My bad on the customisation @swagata87; when we discussed it offline, I missed that it was not needed for one of the modules.

oh no problem!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear where we need to add fillDescriptions to make it work.

From a quick look, it seems that the issue is the lack of a fillDescriptions in CkfTrackCandidateMaker (which in turn would probably be implemented as a function in CkfTrackCandidateMakerBase). That being said, this looks like something beyond the scope of this PR.

@Martin-Grunewald
Copy link
Contributor

+1

@swagata87
Copy link
Contributor Author

It looks like this happens only in Run2 data...
Could it be due to badly measured eta due to pixel failures in 2017?

@akanugan
Copy link
Contributor

akanugan commented Sep 28, 2021

Here are the results from the Validation of this PR:

  1. Few slides about the tests and the list of MINIAOD files (this EEE PR have the extension *_EEE.txt)are in:
    https://cernbox.cern.ch/index.php/s/fC2OxBbPqarIBmG
  2. The validation plots are in:
    https://akanugan.web.cern.ch/akanugan/PF_Validation_EEE_PR35309/

@swagata87
Copy link
Contributor Author

thanks @akanugan for doing the validation,
so we don't see much change in PF objects.. this is expected as eta-extended electrons do not enter particle flow now, it is done via the parameter allowEEEinPF which is set to False. But it's good to see that the switch worked fine.

@jpata
Copy link
Contributor

jpata commented Sep 28, 2021

@swagata87 could you explain this a bit more?

It looks like this happens only in Run2 data...
Could it be due to badly measured eta due to pixel failures in 2017?

Is it that the eta mismeasurement of a GSF track causes the new high-eta hit cut to be used, which creates a slight difference in the GSF collection that PF sees, even with allowEEEinPF=False? The differences are small, but just to make sure we have a reasonable understanding of the mechanism.

@swagata87
Copy link
Contributor Author

Hi Joosep,
my understanding is the following:
with this PR, we are now reconstructing some new GSF tracks at |eta|>2.5 which has <5 hits. But this eta is eta of trajectory. When we are stopping these new GSF tracks (or GED electrons) from entering PF, we are using the condition that, if |eta|>2.5 and nHit<5 then don't let that electron enter PF. But, this eta is not trajectory eta, but this is GED electron's eta. So there will be some cases where GED electron's eta and trajectory eta will not match, and then the new, high eta GED electron might enter PF. I was thinking that, this can happen more often in situations like 2017 data, where there were pixel-failures, which might lead to quite bad mismatch between trajectory eta and GED electron's eta.

@jpata
Copy link
Contributor

jpata commented Sep 29, 2021

+reconstruction

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). 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 Sep 30, 2021

+1

@makortel
Copy link
Contributor

This PR might have caused Trajectory::check() - information requested from empty Trajectory exceptions to fire off in HLT step in several workflows in IBs, see #35488.

@jpata
Copy link
Contributor

jpata commented Oct 1, 2021

For my understanding, would this have been visible from the standard matrix tests somehow, in order to perhaps avoid this in the future? I couldn't see this error in the matrix logs at the moment.

@slava77
Copy link
Contributor

slava77 commented Oct 1, 2021

For my understanding, would this have been visible from the standard matrix tests somehow, in order to perhaps avoid this in the future? I couldn't see this error in the matrix logs at the moment.

IBs include more tests than the short matrix.
You can see the failing tests in IBs from https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/ib/CMSSW_12_1_X
For just 2-3 failures, which are not in the short matrix I think the hard target to fix is the pre-release deadline.

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

10 participants