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

Array ufunc multiplication #1677

Merged
merged 18 commits into from
May 12, 2024

Conversation

andrewgsavage
Copy link
Collaborator

@andrewgsavage andrewgsavage commented Dec 11, 2022

@andrewgsavage andrewgsavage marked this pull request as draft December 11, 2022 13:44
@andrewgsavage andrewgsavage marked this pull request as ready for review December 11, 2022 15:42
Comment on lines 892 to 896
arr_of_q = np.array([Q_(2, "m"), Q_(4, "m")], dtype="object")
q_arr = Q_(np.array([1, 2]), "m")

helpers.assert_quantity_almost_equal(arr_of_q * q_arr, Q_([2, 8], "m^2"))
helpers.assert_quantity_almost_equal(arr_of_q / q_arr, Q_([2, 2], ""))

Choose a reason for hiding this comment

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

This doesn't look quite right to me: You start from an dtype=object array of quantity scalars, where each element could potentially be of a different unit. The result is now a "normal" quantity array, where all elements share a single unit.

Please add a test case for arr_of_q = np.array([Q_(2, "m"), Q_(4, "s")], dtype="object").

Obviously, this will not work with a quantity array, and the expected result of arr_of_q * q_arr would be np.array([Q_(2, "m^2"), Q_(4, "m s")], dtype="object"). For consistency, I would have expected to receive a dtype=object array in all cases.

@hgrecco
Copy link
Owner

hgrecco commented Apr 25, 2023

@andrewgsavage can you comment on @burnpanck objection?

@andrewgsavage
Copy link
Collaborator Author

yea hold merging off till it's addressed

@andrewgsavage
Copy link
Collaborator Author

numpy tests are working on numpy 1.23 but failing on 1.25

I was getting this message for the failing test on 1.23
F:\A\repos\pint\pint\compat.py:306: DeprecationWarning: elementwise comparison failed; this will raise an error in the future.
out = lhs == rhs

@andrewgsavage
Copy link
Collaborator Author

this is ready for review

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 16, 2023

CodSpeed Performance Report

Merging #1677 will degrade performances by 31.5%

Comparing andrewgsavage:array_ufunc_multiplication (02f0209) with master (cb0ec94)

Summary

❌ 1 regressions
✅ 436 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master andrewgsavage:array_ufunc_multiplication Change
test_build_by_mul[mid_array] 12 ms 17.6 ms -31.5%

@andrewgsavage
Copy link
Collaborator Author

Can this be reviewed? I'd like to make pint-pandas changes that rely on this.

Copy link

@burnpanck burnpanck left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't realise that I was listed as a reviewer (probably added automatically by my previous comment). I feel honoured that you are interested in my review, but I'm by no means a maintainer of pint or any related project, so I'm no authority here. I'm just an occasional contributor who likes failures to be loud instead of silent (and thus a big advocate of units libraries in any language).

pint/facets/numpy/numpy_func.py Outdated Show resolved Hide resolved
pint/compat.py Outdated Show resolved Hide resolved
@andrewgsavage
Copy link
Collaborator Author

this is ready for another look

Copy link

@burnpanck burnpanck left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewgsavage andrewgsavage merged commit 7262388 into hgrecco:master May 12, 2024
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operations between arrays of quantity scalars and quantity holding array resulting in incorrect units
4 participants