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

Unified interface for Correction and CompoundCorrection #132

Open
swertz opened this issue May 10, 2022 · 3 comments
Open

Unified interface for Correction and CompoundCorrection #132

swertz opened this issue May 10, 2022 · 3 comments
Labels
enhancement New feature or request evaluator Issues related to the evaluator

Comments

@swertz
Copy link
Contributor

swertz commented May 10, 2022

While testing the jetmet SFs for JES, I realized that working with CompoundCorrections can be a bit awkward. The issue is that depending on the correction level(s) that is (are) needed (simple or multiple), one has to work with either a Correction or a CompoundCorrection object. When writing helper code that loads the corrections, using as input the correction level requested by the user, one doesn't know in advance what will be used. Essentially I need to do e.g.:

// first load all objects
auto cset = CorrectionSet::from_file(".../jet_jerc.json.gz");
std::string key = "Summer19UL18_V5_MC_L2Relative_AK4PFchs"; // or could be "Summer19UL18_V5_MC_L1L2L3Res_AK4PFchs"
auto maybe_corr = std::find_if(cset->begin(), cset->end(), [key](const auto& elem){ return elem.first == key; } );
std::variant<Correction::Ref, CompoundCorrection::Ref> jesSF;
if (maybe_corr !=  cset->end())
    jesSF = maybe_corr->second;
else
    jesSF = cset->compound()[key];
// ...
// then, in the event loop:
float sf;
if (auto corr = std::get_if<Correction::Ref>(&jesSF)) {
    sf = (*corr)->evaluate({eta, pt});
} else {
    sf =  std::get<CompoundCorrection::Ref>(jesSF->evaluate({area,eta,pt,rho});
}

Perhaps it would be easier if both Correction and CompoundCorrection inherited from the same abstract class, that declares the evaluate method?
Then, CorrectionSet::at could automatically return either the correction or the compound correction, depending on the presence of the key in corrections_, dropping the extra compound method?

@nsmith- nsmith- added enhancement New feature or request evaluator Issues related to the evaluator labels May 11, 2022
@nsmith-
Copy link
Collaborator

nsmith- commented May 11, 2022

Hm I wasn't aware there would commonly be a need to switch between simple and compound objects. I don't see an obvious issue with making them polymorphic. (the interior nodes however are purposely not polymorphic)

@swertz
Copy link
Contributor Author

swertz commented May 12, 2022

I would simply argue that whether a correction is compound or not is an implementation detail, that the user does not necessarily need to know about.

@FHead
Copy link

FHead commented May 12, 2022

I second the suggestion. Whether the correction is compound or not is not of interest to the vast majority of users. Internally we can design them as separate, but having a unified user-facing interface would be a very nice enhancement. (Incidentally, this is also one of the points I noticed while working on the JECViewer and mentioned in the Cross-POG meeting last week)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request evaluator Issues related to the evaluator
Development

No branches or pull requests

3 participants