-
Notifications
You must be signed in to change notification settings - Fork 89
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
Pass on skipped v2 tests #1445
Pass on skipped v2 tests #1445
Conversation
Codecov Report
|
4e45827
to
c9bac66
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.
Thanks for this! There must be a lot fewer skips now, though I'd have to check There are no unqualified skips now, apart from the "Missing check for overridden __repr__"
! (There will always be skipif
, for situations like "ROOT was compiled without C++17 support"
.) This is awesome!
% fgrep pytest.mark.skip tests/v2/test_*.py
tests/v2/test_0021-emptyarray.py:@pytest.mark.skip(
tests/v2/test_0025-record-array.py:# @pytest.mark.skip(reason="FIXME: recordtype requires field argument")
tests/v2/test_0032-replace-dressedtype.py:@pytest.mark.skip(reason="Missing check for overridden __repr__")
tests/v2/test_0032-replace-dressedtype.py:@pytest.mark.skip(reason="Missing check for overridden __repr__")
tests/v2/test_0032-replace-dressedtype.py:@pytest.mark.skip(reason="Missing check for overridden __repr__")
tests/v2/test_0032-replace-dressedtype.py:@pytest.mark.skip(reason="Missing check for overridden __repr__")
tests/v2/test_0049-distinguish-record-and-recordarray-behaviors.py:@pytest.mark.skip(reason="Missing check for overridden __repr__")
tests/v2/test_0080-flatpandas-multiindex-rows-and-columns.py:@pytest.mark.skipif(
tests/v2/test_0401-add-categorical-type-for-arrow-dictionary.py:@pytest.mark.skip(reason="Missing check for overridden __repr__")
tests/v2/test_0652-tests-of-complex-numbers.py:@pytest.mark.skip(reason="Remember to implement sorting for complex numbers.")
tests/v2/test_0914-types-and-forms.py:@pytest.mark.skipif(
tests/v2/test_0914-types-and-forms.py:@pytest.mark.skipif(
tests/v2/test_0959-_getitem_array-implementation.py:@pytest.mark.skipif(
tests/v2/test_1125-to-arrow-from-arrow.py:@pytest.mark.skipif(
tests/v2/test_1294-to-and-from_parquet.py:@pytest.mark.skipif(
tests/v2/test_1300-awkward-to-cpp-converter-with-cling.py:@pytest.mark.skipif(not cpp17, reason="ROOT was compiled without C++17 support")
tests/v2/test_1300-awkward-to-cpp-converter-with-cling.py:@pytest.mark.skipif(not cpp17, reason="ROOT was compiled without C++17 support")
tests/v2/test_1300-awkward-to-cpp-converter-with-cling.py:@pytest.mark.skipif(not cpp17, reason="ROOT was compiled without C++17 support")
tests/v2/test_1300-awkward-to-cpp-converter-with-cling.py:@pytest.mark.skipif(not cpp17, reason="ROOT was compiled without C++17 support")
Below, I have just one suggested change (click on it and it will be committed). I had a question about a removed test, but I've answered it: it's okay to remove. So just take the suggested change and then turn on auto-merge (squash).
id = ak._v2.operations.structure.argsort(electron, axis=1) | ||
|
||
assert to_list(v2_electron[id]) == [[], []] | ||
assert v2_electron.typetracer[id].form == v2_electron[id].form |
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.
Yeah, we did just remove record-sorting (and argsorting was never there in v1), so I'm surprised that there's an explicit test for it. I somewhat remember this one—Lindsey contributed it, it was something that came up in Coffea. But I can't see how we ever claimed to sort electron objects, instead of argsorting on electron pt and then applying the integer array to the electron objects.
@lgray, does this test look familiar? Is the record-sorting deprecation that we're talking about in #1451 impact Coffea?
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 did some digging; the first v1 version of this test came in PR #812, which was a bug-fix for issue #811, which @lgray opened, having discovered the bug through Coffea. That's where the references to electrons come from.
In the original issue, only leptons.pt
is passed to argsort
: numbers, not the particle objects. The original issue was about leptons
being a union array, calling .pt
on it and getting a "union of numbers and numbers", calling argsort
on that and getting a "union of integers and integers", and then not being able to use a union array as a slice. That would be a case of a missing simplify_uniontype
, which I think you've addressed in another test here.
Yes, your fix of src/awkward/_v2/contents/unionarray.py is putting a simplify_uniontype
in _getitem_field
(and _getitem_fields
), which would fix issue #811 by making leptons.pt
return a non-union array of numbers.
So I think it's fine to delete this test; it's being tested elsewhere.
Co-authored-by: Jim Pivarski <jpivarski@users.noreply.github.com>
No description provided.