Update imports, use nogil version of sqrt #18557
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18557 +/- ##
==========================================
- Coverage 91.35% 91.33% -0.02%
==========================================
Files 164 164
Lines 49802 49800 -2
==========================================
- Hits 45496 45485 -11
- Misses 4306 4315 +9
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18557 +/- ##
==========================================
+ Coverage 91.44% 91.45% +<.01%
==========================================
Files 157 157
Lines 51449 51447 -2
==========================================
+ Hits 47048 47049 +1
+ Misses 4401 4398 -3
Continue to review full report at Codecov.
|
|
would it not be better to expose this as an definition in a single .pxd, maybe call it cmath.pxd (along with other ones), then simply import centrally from there (or could also just add to util.pxd). |
I see the upside to this but also like transparent/direct imports. Not sure there is enough content to justify a cmath.pxd; what else would go in there? I'll open an issue with cython to see if they can fix the declaration on their end too and make this (eventually) a non-issue. For purposes of expediency, would it be easier if I revert the sqrt bits now and get the easy other fixups closed down? |
|
i would put it in util.pxd the issue is it’s very likely that future changes will accidentally just use the wrong import and just do it with d |
pandas/_libs/window.pyx
Outdated
| @@ -32,11 +32,10 @@ cdef double NaN = <double> np.NaN | |||
| cdef inline int int_max(int a, int b): return a if a >= b else b | |||
| cdef inline int int_min(int a, int b): return a if a <= b else b | |||
|
|
|||
| from util cimport numeric | |||
| from util cimport numeric, sqrt | |||
|
|
|||
| cdef extern from "../src/headers/math.h": | |||
| int signbit(double) nogil | |||
There was a problem hiding this comment.
prob worthwhile adding any of the imports from src/headers/math here as well (prob not very many)
There was a problem hiding this comment.
prob worthwhile adding any of the imports from src/headers/math here as well (prob not very many)
Agreed; its been added to the src todo list. Need to move on for now.
|
conda HTTPError 502 on Appveyor. Will wait until late-evening so re-push as the build queue is usually short then. |
|
looks fine. prefer that you update all of these in this pr. |
|
Travis error in 3.6 TestDataFramePlots.test_parallel_coordinates_with_sorted_labels |
I opened an issue on the cython tracker and based on the response I think we should stick a pin in that until we have a better idea what's going on. |
|
are these in cython 0.26 or greater? |
All my local builds should be on 0.27.1 |
0ec6828 to
e743c1e
Compare
|
and what is the perf affect of this |
The "revert" is reverting from an earlier version of the PR, i.e. keeps the status quo in master unchanged. This should be nothing but import cleanup. |
|
thanks! |
No description provided.