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

add a method to handle optional produces() #46960

Open
fwyzard opened this issue Dec 16, 2024 · 7 comments
Open

add a method to handle optional produces() #46960

fwyzard opened this issue Dec 16, 2024 · 7 comments

Comments

@fwyzard
Copy link
Contributor

fwyzard commented Dec 16, 2024

(inspired by #46453, but encountered a few times already)

Currently it's hard to initialise a token with produces() if the call is optional, for example controlled by another parameter.

It can be done in the body of the constructor, but that prevents the use of const for the token data member:

    if (!isPhase2_) {
      recHitsTokenEE_ = produces(ps.getParameter<std::string>("recHitsLabelEE"));
    }

Ideally we could use something like this in the initialiser list:

    recHitsTokenEE_{isPhase2_ ? device::EDPutToken<OutputProduct>{} : produces(ps.getParameter<std::string>("recHitsLabelEE"))}

but the type returned by produces() is a ProducerBaseAdaptor, not an EDPutToken, so it doesn't work.

Can we add a method like does_not_produce() to be used like

    recHitsTokenEE_{isPhase2_ ? does_not_produce() : produces(ps.getParameter<std::string>("recHitsLabelEE"))}

?

Or even better a method like produces_if() to be used like

    recHitsTokenEE_{produces_if(not isPhase2_, ps.getParameter<std::string>("recHitsLabelEE"))}

?

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 16, 2024

assign core

@cmsbuild
Copy link
Contributor

New categories assigned: core

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

@cmsbuild
Copy link
Contributor

cms-bot internal usage

@cmsbuild
Copy link
Contributor

A new Issue was created by @fwyzard.

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

cms-bot commands are listed here

@makortel
Copy link
Contributor

    recHitsTokenEE_{isPhase2_ ? device::EDPutToken<OutputProduct>{} : produces(ps.getParameter<std::string>("recHitsLabelEE"))}

Passing the output data product type to the produces() call should this form of ternary to work already today, along

    recHitsTokenEE_{isPhase2_ ? device::EDPutToken<OutputProduct>{} : produces<OutputProduct>(ps.getParameter<std::string>("recHitsLabelEE"))}

I acknowledge that suggestion sort of defeats the purpose of the non-typed produces() function though, and therefore isn't really ideal.

We prefer to have this kind functionality in a standalone function rather than extending the interface of edm::ProductRegistryHelper (that would have to propagate to edm::ProducesCollector and possibly elsewhere too). We think the following interface would be doable on a more or less straightforward way

    recHitsTokenEE_{produces_if(not isPhase2_, producesCollector(), ps.getParameter<std::string>("recHitsLabelEE"))}

The same pattern should be extended to ED consumes (in ED modules) and ES consumes (in ED and ES modules).

(Just to note that ProducerBaseAdaptor is the class used in the Alpaka modules, the general one is ProductRegistryHelperAdaptor)

@fwyzard
Copy link
Contributor Author

fwyzard commented Dec 16, 2024

Mhm, the form you propose

recHitsTokenEE_{produces_if(not isPhase2_, producesCollector(), ps.getParameter<std::string>("recHitsLabelEE"))}

is more verbose than

recHitsTokenEE_{produces_if(not isPhase2_, ps.getParameter<std::string>("recHitsLabelEE"))}

and also requires people to learn about producesCollector().

What is the advantage of implementing the former instead of the latter ?

@makortel
Copy link
Contributor

After some further thought we should be able to make the following syntax to work

    recHitsTokenEE_{if_(isPhase2_, produces(ps.getParameter<std::string>("recHitsLabelEE")))}

This approach would be even more composable and likely simpler to implement than the alternative.

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

No branches or pull requests

3 participants