cms-bot internal usage

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

assign RecoEgamma/EgammaElectronProducers

New categories assigned: reconstruction

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

assign upgrade

FYI @cms-sw/egamma-pog-l2

New categories assigned: upgrade

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

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

Any news?

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

@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

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

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

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

type egamma

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.

In a way the behavior prior to #45488 was undefined

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.

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

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

Hi
together with @archiron we are trying to reproduce the problem. Is fast-math enabled as of 14_1_0_pre6 ? Is it possible to enable/disable it at the scram level ? Thanks

together with @archiron we are trying to reproduce the problem. Is fast-math enabled as of 14_1_0_pre6 ?

Yes (I see it was enabled in #13819)

Is it possible to enable/disable it at the scram level ? Thanks

Yes, just remove or comment out the ofast-flag line in BuildFile.xml.

See this PR from @archiron . A few infinity() had been forgotten in the transition.
Running by hand on many previously verbose events, we (@archiron and I) didn't see any computation resulting in infinite (wrong calculations would probably result in nan anyway ?)