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

feat: add boilerplate sub implementation #636

Merged
merged 6 commits into from
Sep 13, 2021

Conversation

amangoel185
Copy link
Contributor

@amangoel185 amangoel185 commented Aug 31, 2021

Basic implementation for #632.

TODO:

  • Add test for scalar
  • Add test for array
  • Add test for Histogram (expected to throw unless Boost.Histogram adds or supports sub)
  • Add test for int storage < 0 after subtraction (unsigned currently) - should throw, convert to double, or storages should be signed.

@github-actions github-actions bot added the needs changelog Might need a changelog entry label Aug 31, 2021
@amangoel185
Copy link
Contributor Author

We need to implement __isub__ but it has to be in accordance with Boost.Histogram

@henryiii
Copy link
Member

henryiii commented Sep 3, 2021

@HDembinski Question for you: should Hist - Hist be supported? I'd only want to support it in boost-histogram if Boost.Histogram supports it, or you have a good reason to support it only in boost-histogram. It's not hard to be explicit - h1 - h2.view(flow=True) for example should do it since subtracting an array should be supported.

Also, maybe we should switch to using signed storage so that subtraction (and negative weights?) work without having to change storages or throw an error. Technically, NumPy uses signed ints, IIRC.

@HDembinski
Copy link
Member

I think Hist should support what boost-histogram supports. The added benefit is that hist - hist can check whether this actually makes sense, it should fail if the axes are not the same.

@henryiii
Copy link
Member

henryiii commented Sep 9, 2021

@HDembinski Sorry, my notation was sloppy. I wasn't suggesting Hist and boost-histogram diverge, I was just asking if a histogram histogram subtraction, bh.Histogram(...) - bh.Histogram(...), should be supported. I was under the impression Boost.Histogram didn't currently support negating and subtracting histograms, but looking at the code, it seems to just fine. So we should add this in boost-histogram!

@henryiii
Copy link
Member

henryiii commented Sep 9, 2021

Okay, needs tests now. @amangoel185 can you look into adding tests as outlined above? Or I might be able to later.

@henryiii
Copy link
Member

henryiii commented Sep 13, 2021

Weighted and AtomicInt64 Boost.Histogram's do not support subtraction, so they don't support subtraction here either.

@henryiii henryiii marked this pull request as ready for review September 13, 2021 03:47
@henryiii
Copy link
Member

@all-contributors please add @amangoel185 for code

@allcontributors
Copy link
Contributor

@henryiii

I've put up a pull request to add @amangoel185! 🎉

@github-actions github-actions bot removed the needs changelog Might need a changelog entry label Sep 13, 2021
@henryiii
Copy link
Member

henryiii commented Sep 13, 2021

After this goes in, I think we should cut a patch release - there have been a few nice fixes, including one holding up a Hist PR, and Python 3.10 support needs to ship.

@henryiii henryiii enabled auto-merge (squash) September 13, 2021 04:04
@henryiii henryiii merged commit df4a6e2 into scikit-hep:develop Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants