-
Notifications
You must be signed in to change notification settings - Fork 4
feat: starting generic testing framework #150
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
Conversation
b128b2c
to
9f4779d
Compare
4d55462
to
d07cae4
Compare
Signed-off-by: Henry Schreiner <[email protected]> [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
d07cae4
to
268db8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (1)
- docs/indexing.rst: Language not supported
I'd like to test the tests (APN-Pucky/babyyoda#60). May I request a new release? |
Have you at run your tests once with the current GitHub install? I'd like to know there aren't bugs in the tests and more exposure does help. :) I'd like to get the serialization stuff done, but I guess that could be broken into two releases, one soon and one once that's in. |
I was waiting for working/proven tests :), but I'll use the current git version then. |
If you can just make sure all the failures (if any) look like they are issues on your side, that would be great. If they are issues in the tests, I'd like to fix them before release - that's how we get working/proven tests. ;) |
@staticmethod | ||
def make_histogram_1() -> bh.Histogram: | ||
h1 = bh.Histogram(bh.axis.Regular(10, 0, 1)) | ||
h1[:] = [0, 2, 4, 6, 8, 10, 12, 14, 16, 18] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this data should be defined in the uhi.testing.indexing.Indexing, right? So that if the testing algorithm changes the data can also be changed only on the UHI side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could, but we also test setting values. I was thinking this could wait till we have serialization support, then we could provide a serialized histogram someone could use if they didn't want to set it up manually.
cls.h1 = cls.make_histogram_1() | ||
cls.h3 = cls.make_histogram_3() | ||
|
||
def test_access_integer_1d(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this directly compares the values (here int).
tests/uhi/generic.py::TestAccess::test_access_integer_1d - AssertionError: GROGU_HISTO1D_V3.Bin(d_sumw=0.0, d_sumw2=[44 chars]=1.0) != 0
In my case the bin also tracks the sumw2 as a members. Would it make sense to wrap the float(self.h1[1])
and my Histo1d bin would default to returning sumw for casting to float?
I am using them now (https://github.com/APN-Pucky/babyyoda/pull/62/files#diff-0f092003db713d939e5d5a829a5bf816f16a4bec1708c796f2b5fc55c0dae36b though I disabled some for 2d case that I was too lazy to getting working for now). |
I like the tests, because indeed some behavior I understood differently from the protocol explanation, compared to the tests. Most notably I thought |
Starting a generic testing helper framework. For a third party to test their implementation, they can subclass the provided UnitTest fixture and implement the required methods producing histograms.
Thoughts so far:
Closes #133.