-
Notifications
You must be signed in to change notification settings - Fork 2
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
Tests #16
Conversation
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 working on this @andrewgsavage !!
tests/test_array.py
Outdated
def test_empty_like(self): | ||
ret = pnp.empty_like(self.q) | ||
assert ret.shape == (2, 2) | ||
assert isinstance(ret, np.ndarray) |
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.
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.
tests/test_array.py
Outdated
# @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)), |
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.
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
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.
self.Q_(0, self.ureg.degC)
is a scalar
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.
the implementation may need changing - I was trying to get the relevant tests running first
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'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.
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.
the failure for pow
is just due to the current implementation being incorrect
I've nothing more to add to this, it's ready for merging |
thanks. I'd like to switch this from using NumPy to using array-api-strict and get everything passing first |
it looks like this will need some tweaking before we are thoroughly testing |
is there a way to say left/right shift operations aren't supported so shouldn't be tested by array-api-tests? |
You can edit |
they're not something you'd want to do a quantity no, they haven't been implemented in pint for np arrays |
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 |
alright, is there anything else that needs doing before this can be merged? |
I need to review it! But yes, I'll skim the diff and see if there is anything obvious. |
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 looked through the changes in the implementation
|
||
mod.meshgrid = _meshgrid | ||
|
||
def _broadcast_arrays(*arrays): |
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.
for a follow-up PR, we should make use a leading underscore consistent everywhere
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. |
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. |
|
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. |
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])