Fix bugs in the Python implementation of inner1d and length #265
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #265 +/- ##
==========================================
- Coverage 81.28% 80.33% -0.96%
==========================================
Files 5 5
Lines 1010 1017 +7
==========================================
- Hits 821 817 -4
- Misses 189 200 +11 ☔ View full report in Codecov by Sentry. |
| @@ -28,7 +28,7 @@ | |||
|
|
|||
|
|
|||
| def _inner1d_np(x, y): | |||
| return np.multiply(x, y).sum(axis=1) | |||
| return np.multiply(x, y).sum(axis=-1) | |||
There was a problem hiding this comment.
So subtle. Does this need a regression test?
There was a problem hiding this comment.
Great idea! I'll do it in a week
|
This does fix the devdeps job, so if you don't want to add a test, feel free to merge. Thanks! |
mcara
force-pushed
the
fix-inner1d-and-length
branch
from
c236a78
to
13d8672
Compare
|
I think this should fix failing tests and the Python versions of Unit tests for would be a plus but no time now. |
| @@ -529,7 +529,7 @@ def test_math_util_angle_domain(): | |||
| ) | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif(math_util is None, reason="math_util C-ext is missing") | |||
| @pytest.mark.skip(reason="temporary turning this off") | |||
There was a problem hiding this comment.
But isn't think just another variation of this that was not accepted?
mcara
force-pushed
the
fix-inner1d-and-length
branch
5 times, most recently
from
76aee62
to
dd631e1
Compare
|
Tests with spherical_geometry/pyproject.toml Line 57 in 7028f15 
|
|
After commit c99e74e most tests (except for one that we skip) with I will now remove tricks used for testing purpose and squash most commits. |
mcara
force-pushed
the
fix-inner1d-and-length
branch
from
7bda523
to
3860cf3
Compare
| @@ -54,7 +54,7 @@ requires = [ | |||
| "setuptools>=61.2", | |||
| "setuptools_scm[toml]>=3.6", | |||
| "numpy>=1.25", | |||
| # "numpy>=2.0.0dev0", | |||
| #"numpy>=2.0.0dev0", | |||
There was a problem hiding this comment.
I think this can be deleted altogether?
| #"numpy>=2.0.0dev0", |
There was a problem hiding this comment.
Well, I left it there for easy switching to numpy 2.0 - that's the reason. Are you asking @jhunkeler ? I hope he has a better solution.
There was a problem hiding this comment.
Oh... this is build requirements. In that case, I think you can just use numpy 2.x like astropy though you might want to pick up RC2 instead of RC1 by now.
https://github.com/astropy/astropy/blob/566fcbd4a36b787564a46b01c4c8322448076d99/pyproject.toml#L133
They claim the 2.x ABI is backward compatible for numpy>=1.23.
xref numpy/numpy#24300 and astropy/astropy#16257
There was a problem hiding this comment.
but if I mention @jhunkeler a couple of times more, we might get an even better solution
There was a problem hiding this comment.
🧟♂️
If the code we have now is backward compatible and our pin is numpy>=1.25, then I suspect it won't be a big deal whenever 2.0 is released. The pins as they are right now (here^) will let tox do what it needs to do. We'll be able to test numpy 1.x and 2.x without any trouble from the build system.
| @@ -54,7 +54,7 @@ requires = [ | |||
| "setuptools>=61.2", | |||
| "setuptools_scm[toml]>=3.6", | |||
| "numpy>=1.25", | |||
There was a problem hiding this comment.
| "numpy>=1.25", | |
| "numpy>=2.0.0rc2", |
mcara
force-pushed
the
fix-inner1d-and-length
branch
from
409cb84
to
b0291e8
Compare
This PR should fix (for now some) errors in unit tests when C
math_utilis not available.length()needs more investigation (but the biggest difference in hownanare treated in comparisons: C code considers twonanequal) and I will do that in about 1 week from now.Fix #261
CC: @jhunkeler @pllim