Noisy ElectronSeedProducer in 2024 and Phase-2 worfklows

[mmusich]

While investigating something else I stumbled upon the fact that ElectronSeedProducer is noisy in several 2024 (but also Phase2) workflows, see e.g.:

• 13034.0: log
• 29634.0: log

The warnings are of the type:

%MSG-w ElectronSeedGenerator:  ElectronSeedProducer:ecalDrivenElectronSeeds  06-Aug-2024 09:03:39 CEST Run: 1 Event: 303
this similar old seed already has another dRz2Pos
old seed mask/dRz2/dPhi2/dRz2Pos/dPhi2Pos: 3/0.00805616/-0.00121471/inf/inf
new seed mask/dRz2/dPhi2/dRz2Pos/dPhi2Pos: 3/inf/inf/0.00778341/-0.000662431
%MSG

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @mmusich.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[makortel]

assign RecoEgamma/EgammaElectronProducers

[cmsbuild]

New categories assigned: reconstruction

@jfernan2,@mandrenguyen you have been requested to review this Pull request/Issue and eventually sign? Thanks

[makortel]

assign upgrade

[makortel]

FYI @cms-sw/egamma-pog-l2

[cmsbuild]

New categories assigned: upgrade

@srimanob,@subirsarkar you have been requested to review this Pull request/Issue and eventually sign? Thanks

[jfernan2]

@sameasy @Prasant1993 as E-gamma reco contacts, could you please have a look? Thanks

[makortel]

Any news?

[RSalvatico]

Shamik and Prasant are not EGM reco conveners anymore. Hopefully @SanghyunKo can have a look?

[jfernan2]

@RSalvatico if those persons are not EGM Reco contacts any longer, I believe we need to update the names in [1], should I add @SanghyunKo ?

[1] https://twiki.cern.ch/twiki/bin/view/CMS/RecoContacts

[RSalvatico]

@jfernan2 I have added to the twiki the names, email addresses, and GitHub usernames of the new EGM Reco conveners

[rovere]

I think this PR caused the triggering of so many messages:
#45488

I tag @VinInn as the expert on this matter (approx_log)

[mmusich]

type egamma

[beaudett]

This PR has an other consequence, which was reported here
(The initial report was issued before the link with the PR has been established)
Namely, the number of electron seeds made with the q<0 or q>0 assumption became different. Here is the plot showing the difference for the q>0 hypothesis. pre6 is in red, pre5 in blue. The plot is normalized, but one can see from the number of entries that we lost many seeds between the two releases.
The equivalent plot with the q<0 hypothesis is here

I assume that the error message issued in the logs corresponds to one seed which is lost.
Fortunately enough, it does not impact the electron efficiency. It might affect the fake rate though but it hasn't been checked.
It has been checked (thanks @archiron and @AdrianoDee ) that if the PR is discarded one gets back the seeds, see here (red is pre6 without the PR, blue is pre5, and we are looking at the q>0 hypothesis)

How the above PR changed the seed reconstruction is so far a mystery to me.

[makortel]

In a way the behavior prior to #45488 was undefined

[beaudett]

I am not denying that, even though I am not an expert of this code. We will try to have a deeper look at the behavior of this piece of code before and after the change.

[makortel]

So the printout

%MSG-w ElectronSeedGenerator:  ElectronSeedProducer:ecalDrivenElectronSeeds  06-Aug-2024 09:03:39 CEST Run: 1 Event: 303
this similar old seed already has another dRz2Pos
old seed mask/dRz2/dPhi2/dRz2Pos/dPhi2Pos: 3/0.00805616/-0.00121471/inf/inf
new seed mask/dRz2/dPhi2/dRz2Pos/dPhi2Pos: 3/inf/inf/0.00778341/-0.000662431
%MSG

comes from

cmssw/RecoEgamma/EgammaElectronAlgos/src/ElectronSeedGenerator.cc

Lines 73 to 82 in 8340b52

	if (res.dRZPos(1) != std::numeric_limits<float>::max()) {
	if (res.dRZPos(1) != seed.dRZPos(1)) {
	edm::LogWarning("ElectronSeedGenerator|BadValue")
	<< "this similar old seed already has another dRz2Pos"
	<< "\nold seed mask/dRz2/dPhi2/dRz2Pos/dPhi2Pos: " << (unsigned int)res.hitsMask() << "/"
	<< res.dRZNeg(1) << "/" << res.dPhiNeg(1) << "/" << res.dRZPos(1) << "/" << res.dPhiPos(1)
	<< "\nnew seed mask/dRz2/dPhi2/dRz2Pos/dPhi2Pos: " << (unsigned int)seed.hitsMask() << "/"
	<< seed.dRZNeg(1) << "/" << seed.dPhiNeg(1) << "/" << seed.dRZPos(1) << "/" << seed.dPhiPos(1);
	}
	}

According to the printout res.dRZPos(1) and res.dPhiPos(1) are inf. Earlier the comparison on line 73 was against std::numeric_limits<float>::infinity(), and a different branch was taken.

I'd guess some computations result in infinities. Maybe these comparisons that were earlier against infinity() and are now against max() would need to be accompanied with edm::isFinite() check.

(makes me also wonder how safe use of the fast-math is here...)