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

Tests #16

Merged
merged 52 commits into from
Jan 19, 2025
Merged

Tests #16

merged 52 commits into from
Jan 19, 2025

Conversation

andrewgsavage
Copy link
Contributor

adds some relevant tests copied from pint's numpy tests. probably adds some that aren't using pint-array too.

FAILED tests/test_array.py::TestNumpyArrayCreation::test_empty_like - AssertionError: assert False
FAILED tests/test_array.py::TestNumpyArrayCreation::test_full_like - AssertionError: Comparing <Quantity(
FAILED tests/test_array.py::TestNumpyArrayManipulation::test_transpose - AssertionError: Comparing array([[1, 3],
FAILED tests/test_array.py::TestNumpyArrayManipulation::test_concat_stack - pint.errors.DimensionalityError: Cannot convert from 'meter' ([length]) to 'dimensionless' (dimensionless)
FAILED tests/test_array.py::TestNumpyArrayManipulation::test_concat_stack - pint.errors.DimensionalityError: Cannot convert from 'meter' ([length]) to 'dimensionless' (dimensionless)
FAILED tests/test_array.py::TestNumpyArrayManipulation::test_broadcast_arrays - pint.errors.DimensionalityError: Cannot convert from 'dimensionless' to 'nanometer'
FAILED tests/test_array.py::TestNumpyMathematicalFunctions::test_prod_numpy_func - AssertionError: Comparing <Quantity(
FAILED tests/test_array.py::TestNumpyMathematicalFunctions::test_power - Failed: DID NOT RAISE <class 'pint.errors.DimensionalityError'>
FAILED tests/test_array.py::TestNumpyUnclassified::test_max_with_initial_arg - pint.errors.DimensionalityError: Cannot convert from 'meter' to 'dimensionless'
FAILED tests/test_array.py::TestNumpyUnclassified::test_min_with_initial_arg - pint.errors.DimensionalityError: Cannot convert from 'meter' to 'dimensionless'
FAILED tests/test_array.py::TestNumpyUnclassified::test_meshgrid_numpy_func - AttributeError: 'numpy.ndarray' object has no attribute 'm'
FAILED tests/test_array.py::TestNumpyUnclassified::test_where - pint.errors.DimensionalityError: Cannot convert from 'dimensionless' (dimensionless) to 'meter' ([length])

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

thanks for working on this @andrewgsavage !!

Comment on lines 75 to 78
def test_empty_like(self):
ret = pnp.empty_like(self.q)
assert ret.shape == (2, 2)
assert isinstance(ret, np.ndarray)
Copy link
Member

Choose a reason for hiding this comment

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

this is a place where we will differ from the existing pint + NumPy behaviour. Any function which returns an array in the spec (e.g. https://data-apis.org/array-api/2023.12/API_specification/generated/array_api.empty_like.html#empty-like) will return a Quantity here.

So in most cases we will return a dimensionless quantity instead. But I know you already mentioned that the behaviour of the *_like functions should probably change to match the units of the input.

# @helpers.requires_array_function_protocol()
def test_full_like(self):
helpers.assert_quantity_equal(
pnp.full_like(self.q, self.Q_(0, self.ureg.degC)),
Copy link
Member

Choose a reason for hiding this comment

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

in the spec, the fill_value argument must be a python scalar, not an array - https://data-apis.org/array-api/2023.12/API_specification/generated/array_api.full_like.html#full-like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.Q_(0, self.ureg.degC) is a scalar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the implementation may need changing - I was trying to get the relevant tests running first

Copy link
Member

Choose a reason for hiding this comment

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

it's a Quantity (scalar + unit), right? Which in terms of the standard is a 0-dimensional array, as opposed to a python scalar. Whether or not the standard is correct here has been questioned at data-apis/array-api-strict#55.

tests/test_array.py Outdated Show resolved Hide resolved
tests/test_array.py Outdated Show resolved Hide resolved
tests/test_array.py Outdated Show resolved Hide resolved
tests/test_array.py Outdated Show resolved Hide resolved
tests/test_array.py Outdated Show resolved Hide resolved
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

the failure for pow is just due to the current implementation being incorrect

tests/test_array.py Outdated Show resolved Hide resolved
@andrewgsavage
Copy link
Contributor Author

I've nothing more to add to this, it's ready for merging

@lucascolley
Copy link
Member

thanks. I'd like to switch this from using NumPy to using array-api-strict and get everything passing first

@lucascolley
Copy link
Member

it looks like this will need some tweaking before we are thoroughly testing pxp arrays rather than np arrays, since list * unit gives a NumPy array.

@andrewgsavage
Copy link
Contributor Author

is there a way to say left/right shift operations aren't supported so shouldn't be tested by array-api-tests?

@lucascolley
Copy link
Member

You can edit xp-tests-xfails.txt. What is the problem though? Are they not well-defined for dimensionless quantities?

@andrewgsavage
Copy link
Contributor Author

they're not something you'd want to do a quantity no, they haven't been implemented in pint for np arrays

@lucascolley
Copy link
Member

they're not something you'd want to do a quantity no

Okay, that's fine for now, but long term I'm not convinced it is the right solution to stray from the standard for dimensionless arrays. It would be ideal if the result of asarray with no units specified worked like a normal array. I guess we can see in time how much of a problem it causes in downstream libraries, and reconsider if it is something they need.

@andrewgsavage
Copy link
Contributor Author

alright, is there anything else that needs doing before this can be merged?

@lucascolley
Copy link
Member

I need to review it! But yes, I'll skim the diff and see if there is anything obvious.

Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

I looked through the changes in the implementation

xp-tests-xfails.txt Outdated Show resolved Hide resolved
src/pint_array/__init__.py Outdated Show resolved Hide resolved
src/pint_array/__init__.py Show resolved Hide resolved
src/pint_array/__init__.py Outdated Show resolved Hide resolved

mod.meshgrid = _meshgrid

def _broadcast_arrays(*arrays):
Copy link
Member

Choose a reason for hiding this comment

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

for a follow-up PR, we should make use a leading underscore consistent everywhere

src/pint_array/__init__.py Show resolved Hide resolved
src/pint_array/__init__.py Outdated Show resolved Hide resolved
src/pint_array/__init__.py Outdated Show resolved Hide resolved
src/pint_array/__init__.py Outdated Show resolved Hide resolved
@andrewgsavage
Copy link
Contributor Author

do you have a reference for needing to return the same array type? indicies and bools aren't quantities so dimensionless quantities doesn't make sense.
pandas EAs are required to return ints or bools for these operations
How do arrays that don't store ints/bools deal with this?

@lucascolley
Copy link
Member

lucascolley commented Jan 12, 2025

do you have a reference for needing to return the same array type? indicies and bools aren't quantities so dimensionless quantities doesn't make sense.
pandas EAs are required to return ints or bools for these operations

Hmm, there may not be a requirement for this in the standard, now that I take a look. I think we do assume it in SciPy though. At least in tests, we check that input namespaces are equal to output namespace, so that machinery might need to be made smarter to work with a wrapping library which can return arrays of the underlying namespace instead.

Happy to roll with your way for now and see if we hit any problems.

@lucascolley
Copy link
Member

lucascolley commented Jan 12, 2025

How do arrays that don't store ints/bools deal with this?

bool, int32 and int64 are required dtypes in the standard. There has been discussion on relaxing this and figuring out how to deal with missing dtypes, mainly because Apple's MLX library does not have double precision dtypes. But nothing has come of that yet. I can't foresee there being any accommodation for missing boolean dtype though. And there probably needs to be at least one integer dtype.

@lucascolley lucascolley self-requested a review January 19, 2025 12:05
@lucascolley
Copy link
Member

thanks again for working on this @andrewgsavage ! I pushed some tweaks, and will merge soon.

I'll make some more tweaks, like changing the project name to quantity-array, then look at setting up continuous deployment, and making a first release.

@lucascolley lucascolley merged commit 1a29dd9 into quantity-dev:main Jan 19, 2025
7 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