-
Notifications
You must be signed in to change notification settings - Fork 478
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
Array ufunc multiplication #1677
Conversation
pint/testsuite/test_issues.py
Outdated
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], "")) |
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 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.
@andrewgsavage can you comment on @burnpanck objection? |
yea hold merging off till it's addressed |
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 |
this is ready for review |
CodSpeed Performance ReportMerging #1677 will degrade performances by 31.5%Comparing Summary
Benchmarks breakdown
|
Can this be reviewed? I'd like to make pint-pandas changes that rely on this. |
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.
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).
this is ready for another look |
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.
LGTM
pre-commit run --all-files
with no errors