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

TST: test/fix astype/clip #20

Merged
merged 4 commits into from
Dec 1, 2024
Merged

TST: test/fix astype/clip #20

merged 4 commits into from
Dec 1, 2024

Conversation

mdhaber
Copy link
Owner

@mdhaber mdhaber commented Nov 24, 2024

Grouping these two because because it was convenient to do so. Pretty straightforward.

@mdhaber mdhaber changed the title TST: test/fix astype TST: test/fix astype/clip Nov 24, 2024
datas = [getattr(arg, 'data', arg) for arg in args]
data = xp.clip(datas[0], min=datas[1], max=datas[2])
return MaskedArray(data, mask)
mod.clip = clip
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be done in a new PR, but I believe it would be neater to use a decorator for this

Copy link
Owner Author

Choose a reason for hiding this comment

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

You mean for adding the attribute to the namespace? What is the advantage?

marrays, masked_arrays, seed = get_arrays(1, dtype=dtype_in, seed=seed)

if dtype_in != dtype_out and not copy:
pytest.mark.skip("Can't change type without copy.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this actually do anything? I believe we need to use pytest.skip instead:

Suggested change
pytest.mark.skip("Can't change type without copy.")
pytest.skip(reason="Can't change type without copy.")

Copy link
Owner Author

@mdhaber mdhaber Nov 24, 2024

Choose a reason for hiding this comment

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

No, it was a typo, and I guess it wasn't needed. Removed.

marray/tests/test_marray.py Outdated Show resolved Hide resolved
@mdhaber mdhaber requested a review from keewis November 25, 2024 00:10
@mdhaber
Copy link
Owner Author

mdhaber commented Nov 25, 2024

Interesting - the test caught what is arguably an edge case bug in NumPy's masked arrays:

np.any(np.ma.masked_array([-1], mask=[True])).dtype  # dtype('float64')

I guess it's because:

type(np.any(np.ma.masked_array([-1], mask=[True])))
# numpy.ma.core.MaskedConstant

TBH I think MaskedConstant was just one big mistake. Well, as was reducing 1-D arrays to scalars in general.... But hindsight is 20/20.

Anyway, the test failure does not indicate a bug in this PR, so nothing to fix here. If this comes up frequently, we can adjust the way the reference of that test is computed, but in the meantime there are better ways to spend time.

@@ -521,11 +521,39 @@ def test_searchsorted(side, xp=strict, seed=None):
# Test Linear Algebra functions

# Use Array API tests to test the following:
# Data Type Functions (only `astype` remains to be tested)
# Elementwise function `clip` (all others are tested above)
# Creation Functions (same behavior but with all-False mask)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Suggested change
# Creation Functions (same behavior but with all-False mask)

@mdhaber mdhaber merged commit d1ac508 into main Dec 1, 2024
12 checks passed
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.

2 participants