fix: proper comparison in ak.almost_equal between record array fields#3962
Conversation
|
@TaiSakuma I'm surprised hypothesis-awkward did not find this in CI yet. I magically found it my running the property-based tests in parallel threads 🤣. |
ak.almost_equal between record arraysak.almost_equal between record array fields
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3962 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
|
@ikrommyd The existing property-based tests on To find this with hypothesis-awkward, we will need tests like: @given(a=st_ak.constructors.arrays())
def test(a: ak.Array) -> None:
b = change_field_name(a)
assert not ak.array_equal(a, b, equal_nan=True)Which tests are "the property-based tests in parallel threads"? Are they in the repo? |
|
Yes we do run the whole CI with 4 parallel threads but the error hasn't been triggered in CI so far. awkward/.github/workflows/test.yml Lines 161 to 165 in f44402c I randomly got it locally by doing pytest tests/properties --parallel-threads 4 (you need pytest-run-parallel installed)
|
|
For reference, this is what I got on main |
|
@ikrommyd I see. It is not the assertion that failed. I think that |
|
Yeah but as we keep increasing the property based tests, if we also increase max examples, it will take too long. |
|
Yes. We can keep |
ianna
left a comment
There was a problem hiding this comment.
Is converting index to field intentional/desired? The tuple RecordArray does not have fields.
I don't undertand the question. The tuple record array does internally "have" fields that are just integers ("0", "1"....). The are not saved in ._fields but that's how awkward keeps track of which "field" is which. By numbering them. And you can actually see them with |
|
These were the errors of the tests without this PR A) different number of fields would not be compared properly in the tuple case |
<RecordArray is_tuple='true' len='2'>
<content index='0'>
<NumpyArray dtype='float64' len='2'>[1. 3.]</NumpyArray>
</content>
<content index='1'>
<NumpyArray dtype='int64' len='2'>[2 4]</NumpyArray>
</content>
</RecordArray>vs <RecordArray is_tuple='false' len='1'>
<content index='0' field='x'>
<NumpyArray dtype='int64' len='1'>[2]</NumpyArray>
</content>
<content index='1' field='y'>
<NumpyArray dtype='int64' len='1'>[4]</NumpyArray>
</content>
</RecordArray> |
|
Yeah and what about those two. Tuple vs non-tuple the equality check should return False even if each column is the same because manually named fields have a meaning to the user while how awkward internally labels fields is an arbitrary choice of awkward. A manual "0", "1", "2" labeling means the user explicitly chose to name those fields like that. That fact that it randomly coincides with how awkward internally chooses to label things in the tuple case is not relevant. |
Is converting |
I still don't understand, there is no conversion added here in this PR. That's just what awkward does since forever. Are we talking about policy here or a specific change? |
Hypothesis settings profiles replace the hardcoded per-test max_examples: "default" (200) for PR CI and local runs, "nightly" (10,000) selected via the HYPOTHESIS_PROFILE environment variable. A new scheduled workflow runs tests/properties nightly at the large profile, plus a second pass under pytest-run-parallel (4 threads), which is how the bug behind #3962 was originally found. A README badge surfaces nightly failures; the stale Build Test badge (dead since the rename in #3969) now points at ci.yml. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Yet another
ak.almost_equalproblem regarding comparing record arrays this time.Comparing tuple vs non-tuple record arrays and the number of names of fields within them was not done right.