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

Upper bin edge included when evaluating JEC when using non-linear lookup #34381

Closed
kirschen opened this issue Jul 7, 2021 · 7 comments
Closed

Comments

@kirschen
Copy link
Contributor

kirschen commented Jul 7, 2021

Discovered an inconsistency between the current non-linear lookup of bin indices and both

  1. the previous "legacy" linear index search JetCorrectorParameters::binIndex()
  2. the correctionlib implementation for interpreting the JSON files

It seems like this boils down to including the upper bin edge in the non-linear search


vs. excluding it in the linear search and correctionlib
if (fX[j] >= record(i).xMin(j) && fX[j] < record(i).xMax(j))

@aperloff : Can you confirm? Will put in a tiny PR fixing the inclusion of the upper bin edge it but would be good to get your confirmation.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2021

A new Issue was created by @kirschen Henning Kirschenmann.

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@kirschen
Copy link
Contributor Author

kirschen commented Jul 7, 2021

And just for reference: This really only affects the outermost upper bin edge (e.g. for eta-binned corrections usually eta=5.191), so just a nuance, but better to fix if it's easily fixable (which it is).
FYI: @juska @tankit @camclean @lathomas @nsmith-

@makortel
Copy link
Contributor

makortel commented Jul 7, 2021

assign db

FYI @cms-sw/reconstruction-l2 (not sure who else should be made aware)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2021

New categories assigned: db

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

@ggovi
Copy link
Contributor

ggovi commented Jul 15, 2021

addressed by #34382 ?

@perrotta
Copy link
Contributor

perrotta commented Aug 4, 2021

@kirschen please confirm that the issue is solved by #34382, and close this github issue if so

@kirschen
Copy link
Contributor Author

Indeed solved by #34382. Closing...

kirschen added a commit to cms-jet/JECDatabase that referenced this issue Aug 24, 2021
- Fix issue at upper bin edge of outermost bins (inconsistency in CMSSW-code, cf. cms-sw/cmssw#34381)
- Set name/description such that multiple JSONs could be merged
- replace TMath::Log automatically, but other uses of TMath:XXX will still break
- Update README to reflect new functionality
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

5 participants