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 UniformBinning to schemav2 #186

Merged
merged 7 commits into from
Jul 19, 2023
Merged

Conversation

eguiraud
Copy link
Contributor

@eguiraud eguiraud commented May 16, 2023

When defining a Binning, the edges attribute can now be either a list of floats (like before) or an instance of UniformBinning(n, low, high). When using the latter option, the Binning correction is optimized to use a simple arithmetic formula rather than binary search to look up the bin index.

This is a fresh (better) take on #181 .

Example usage:

from correctionlib import schemav2 as schema

if __name__ == "__main__":
    corr = schema.Correction(
        name="test",
        version=2,
        inputs=[schema.Variable(name="x", type="real")],
        output=schema.Variable(name="a scale", type="real"),
        data=schema.Binning(
            nodetype="binning",
            input="x",
            edges=schema.UniformBinning(n=2, low=0., high=2.),
            content=[1.0, 2.0],
            flow="error",
        ),
    )
    e = corr.to_evaluator()
    print(e.evaluate(0.5))
    print(e.evaluate(1.0))
    print(e.evaluate(3.0)) # errors out

To do:

  • add tests
  • add support for MultiBinning

@eguiraud eguiraud force-pushed the uniform-binning branch 2 times, most recently from 579f7aa to 42bc652 Compare May 16, 2023 20:35
When defining a Binning, the `edges` attribute can now
be either a list of floats (like before) or an instance
of UniformBinning(n, low, high). When using the latter
option, the Binning correction is optimized to use a
simple arithmetic formula rather than binary search to
look up the bin index.
Copy link
Collaborator

@nsmith- nsmith- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is the right approach I think.
Looks good, some small extra validation comments only

src/correction.cc Outdated Show resolved Hide resolved
src/correction.cc Show resolved Hide resolved
@eguiraud
Copy link
Contributor Author

eguiraud commented Jun 8, 2023

Addressed review comments, added some tests for Binning+UniformBinning, fixed a bug in an edge case that the test found.

I will work on adding support for MultiBinning as well as soon as possible (could also be a separate PR).

@nsmith-
Copy link
Collaborator

nsmith- commented Jun 9, 2023

I'm fine to wait for MultiBinning in this PR

Copy link
Collaborator

@nsmith- nsmith- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks so much for this addition, and sorry I didn't review it sooner

@nsmith- nsmith- merged commit 8048c11 into cms-nanoAOD:master Jul 19, 2023
@eguiraud eguiraud deleted the uniform-binning branch July 19, 2023 16:39
@nsmith- nsmith- linked an issue Jul 19, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Optimize bin lookup for uniform binning
2 participants