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

Event::getByToken bug in CutBasedElectronID #46866

Open
Dr15Jones opened this issue Dec 4, 2024 · 9 comments · May be fixed by #46868
Open

Event::getByToken bug in CutBasedElectronID #46866

Dr15Jones opened this issue Dec 4, 2024 · 9 comments · May be fixed by #46868

Comments

@Dr15Jones
Copy link
Contributor

The lines

edm::Handle<reco::BeamSpot> pBeamSpot;
// uses the same name for the vertex collection to avoid adding more new names
e.getByToken(verticesCollection_, pBeamSpot);

will always result in a failure because the type of verticesCollection_ does not match BeamSpot

edm::EDGetTokenT<std::vector<reco::Vertex> > verticesCollection_;

Such a mismatch was intended to be caught at compile time but didn't happen because the compiler found the overload

template <typename PROD>
bool getByToken(EDGetToken token, Handle<PROD>& result) const;

and used that.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2024

A new Issue was created by @Dr15Jones.

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

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor Author

assign RecoEgamma/ElectronIdentification

@Dr15Jones
Copy link
Contributor Author

assign core

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2024

New categories assigned: reconstruction,core

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

@Dr15Jones
Copy link
Contributor Author

A bit of git archeology shows this has been broken for 10 years.

@jfernan2
Copy link
Contributor

jfernan2 commented Dec 4, 2024

@SanghyunKo @cochando as L3 Reco EGamma conveners, can you please have a look and provide a solution? Thanks

@Dr15Jones
Copy link
Contributor Author

I actually have a fix for this one coming as part of a general memory improvement I'm doing to ElectronIDExternalProducer and its helpers.

@Dr15Jones Dr15Jones linked a pull request Dec 4, 2024 that will close this issue
@makortel
Copy link
Contributor

makortel commented Dec 4, 2024

Such a mismatch was intended to be caught at compile time but didn't happen because the compiler found the overload

template <typename PROD>
bool getByToken(EDGetToken token, Handle<PROD>& result) const;

and used that.

Maybe we should finally remove the non-templated EDGetToken (and EDPutToken) (we have already thought about it cms-sw/framework-team#431)

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

Successfully merging a pull request may close this issue.

4 participants