ENH: implement nep 0015: merge multiarray and umath #10915
Conversation
| PyObject *c_api = NULL; | ||
|
|
||
| if (numpy == NULL) { | ||
| PyErr_SetString(PyExc_ImportError, "numpy.core.umath failed to import"); | ||
| numpy = PyImport_ImportModule("_multiarray"); |
There was a problem hiding this comment.
Why are we trying to import a top-level module?
There was a problem hiding this comment.
see comment below, maybe we don't need this method at all?
There was a problem hiding this comment.
I don't have enough experience here to know if we want this method.
I'm just worried that this looks like a removed-in-py3k implicit relative import.
There was a problem hiding this comment.
removed. It was useful only to import _multiarray indepedent of the rest of numpy:
data = imp.find_module('_multiarray', ['cpython3_virt/lib/python3.5/site-packages/numpy/core'])
multiarray = imp.load_module('_multiarray', *data)
which I was doing for debugging the bootstrap of numpy.init
numpy/core/multiarray.py
Outdated
| 'where', 'zeros'] | ||
|
|
||
| for s in __all__: | ||
| globals()[s] = getattr(_multiarray, s) |
There was a problem hiding this comment.
Given you already have the import * above, do you really need this? I'd be inclined to drop the import * and stick with this.
| @@ -46,11 +46,14 @@ | |||
| _import_array(void) | |||
| { | |||
| int st; | |||
| PyObject *numpy = PyImport_ImportModule("numpy.core.multiarray"); | |||
| PyObject *numpy = PyImport_ImportModule("numpy.core._multiarray"); | |||
There was a problem hiding this comment.
Maybe rename to _multiarray_umath or something with both names, to make it clearer for people who already know about multiarray existing?
|
about
The second test seems redundant. The first is now redundant in numpy since they are one-in-the-same module. The third can be moved to Am I missing something? |
9cc4aca to
3703c72
Compare
|
Still WIP. I did not adapt |
numpy/core/setup.py
Outdated
| ####################################################################### | ||
| # umath module # | ||
| # _multiarray_umath module - multiarray part # |
numpy/core/setup.py
Outdated
| join('src', 'private', 'mem_overlap.c'), | ||
| join('src', 'private', 'npy_longdouble.c'), | ||
| join('src', 'private', 'ufunc_override.c')] | ||
| # join('src', 'private', 'mem_overlap.c'), |
There was a problem hiding this comment.
These disappear because they are already included above, correct?
| @@ -36,11 +36,14 @@ | |||
| static NPY_INLINE int | |||
| _import_umath(void) | |||
| { | |||
| PyObject *numpy = PyImport_ImportModule("numpy.core.umath"); | |||
| PyObject *numpy = PyImport_ImportModule("numpy.core._multiarray"); | |||
There was a problem hiding this comment.
I'm very confused, shouldn't it be _multiarray_umath here and below?
3703c72 to
00e3433
Compare
|
Just to note that I want to branch 1.15 before this goes in. |
|
This is more an experiment in open development, I expect it will take quite a while till I get all the pieces in place. Maybe I should close the PR and continue pushing to my fork to reduce noise here |
|
@mattip - I think it is very good to see it happening, especially as this is quite an involved PR. I quite like the approach of doing it as much as possible in small pieces. Anyway, I would not worry about the "noise" - you warned us amply that it was a WIP! Just ping when it is useful to look again. |
88b21fe to
28ce0b4
Compare
28ce0b4 to
010fc4f
Compare
|
Rebased off master after 1.15 |
|
I'm quite excited about this - do ping if this is ready for further review! |
010fc4f to
b997efd
Compare
|
I took the WIP out of the title and am ready for a review. While the NEP (PR #10704) itself has not progressed, I have implemented the minimal set of changes I could to merge the two c-extension modules. Merging this would be helpful for continuing the work on matmul (PR #11133) since it would give access to the Note that I only moved files that have not changed in quite a while and that have no |
numpy/core/setup.py
Outdated
| join('src', 'private', 'mem_overlap.c'), | ||
| join('src', 'private', 'npy_longdouble.c'), | ||
| join('src', 'private', 'ufunc_override.c')] | ||
| # join('src', 'common', 'mem_overlap.c'), |
There was a problem hiding this comment.
Why the commented out files? Just remove? Or at least comment on why they are commented out... Is it because they are already listed in multiarray?
numpy/core/setup.py
Outdated
| join('src', 'common', 'binop_override.h')] + npymath_sources | ||
|
|
||
| config.add_extension('_multiarray_umath', | ||
| sources=multiarray_src + umath_src + |
There was a problem hiding this comment.
Might it be an idea to have a common_src here?
There was a problem hiding this comment.
adopted. Eventually everything will become common
| #include "numpy/ufuncobject.h" | ||
| #include "ufunc_object.h" | ||
| /* | ||
| ***************************************************************************** |
There was a problem hiding this comment.
Might it be more logical to have this contained in a separate umathmodule.h file?
There was a problem hiding this comment.
It seems there may be unused includes, reworking
There was a problem hiding this comment.
I cleaned this up by removing many uneeded include directives
| @@ -4277,6 +4294,11 @@ normalize_axis_index(PyObject *NPY_UNUSED(self), PyObject *args, PyObject *kwds) | |||
| return PyInt_FromLong(axis); | |||
| } | |||
|
|
|||
| /* declarations from umathmodule.c */ | |||
There was a problem hiding this comment.
If following the separate umathmodule.h file idea above, these lines could go in there too.
There was a problem hiding this comment.
cleanup up via removal and refactoring, without a umathmodule.h
There was a problem hiding this comment.
on second thought umathmodule.h would be better
numpy/core/src/umath/umathmodule.c
Outdated
| } | ||
|
|
||
| /* Import the array */ | ||
| /* No need to import the array, only called via import in the first place |
There was a problem hiding this comment.
Given the comment, should this be removed?
numpy/core/setup.py
Outdated
| join('src', 'common', 'ufunc_override.h'), | ||
| join('src', 'common', 'binop_override.h'), | ||
| join('src', 'common', 'npy_extint128.h'), | ||
| join('src', 'common', 'lowlevel_strided_loops.h'), |
There was a problem hiding this comment.
From this one on down, these are duplications.
de16ebe to
bb2cc21
Compare
|
rebased off master to solve merge conflicts, added common/umathmodule.h |
bb2cc21 to
9246b07
Compare
a6bf7e9 to
03e0814
Compare
|
Rebased off master to see if gcov messages noted in #11790 go away after this PR. Also properly formatted commit messages and sqaushed a few together. |
|
|
| join('src', 'private', 'npy_extint128.h')], | ||
| join('src', 'common', 'mem_overlap.c')], | ||
| depends=[join('src', 'common', 'mem_overlap.h'), | ||
| join('src', 'common', 'npy_extint128.h')], |
There was a problem hiding this comment.
I'm able to reproduce the gcov issue locally on linux & moving this extension addition block up above _multiarray_umath module prevents mem_overlap.c from being excluded from the C coverage report.
The diff is below and build / unit tests still seem fine locally for me:
diff --git a/numpy/core/setup.py b/numpy/core/setup.py
index b306aa4..1588a26 100644
--- a/numpy/core/setup.py
+++ b/numpy/core/setup.py
@@ -710,6 +710,17 @@ def configuration(parent_package='',top_path=None):
include_dirs=[])
#######################################################################
+ # multiarray_tests module #
+ #######################################################################
+
+ config.add_extension('_multiarray_tests',
+ sources=[join('src', 'multiarray', '_multiarray_tests.c.src'),
+ join('src', 'common', 'mem_overlap.c')],
+ depends=[join('src', 'common', 'mem_overlap.h'),
+ join('src', 'common', 'npy_extint128.h')],
+ libraries=['npymath'])
+
+ #######################################################################
# _multiarray_umath module - common part #
#######################################################################
@@ -933,16 +944,6 @@ def configuration(parent_package='',top_path=None):
config.add_extension('_struct_ufunc_tests',
sources=[join('src', 'umath', '_struct_ufunc_tests.c.src')])
- #######################################################################
- # multiarray_tests module #
- #######################################################################
-
- config.add_extension('_multiarray_tests',
- sources=[join('src', 'multiarray', '_multiarray_tests.c.src'),
- join('src', 'common', 'mem_overlap.c')],
- depends=[join('src', 'common', 'mem_overlap.h'),
- join('src', 'common', 'npy_extint128.h')],
- libraries=['npymath'])I could open a PR to do this on master, but since @mattip has been working on this code for a while I'll let him judge if this might be suitable & avoids merged conflicts if we change here, etc.
|
This seems a bit magical. Any ideas why it solves the problem? I don't mind making the change but perhaps we are simply masking a deeper issue. For instance, and this is just a theory I am making up, it may be that the gcov report for one extension module is getting silently written over by the other, where before the change at least we got the warning. |
|
I'm not sure I follow -- the Can you explain why it might be bad to reverse the extension building order in the way that I have? Why was it the other way around initially? It looks like the idea might be to construct functionality / libraries first, and then tests second, but why do tests need to duplicate what was built for functionality in a well-designed system? We could probably dissect the timestamps for all object files and gcov report files that are involved to figure out the reason for warning suppression on order reversal; perhaps invoking gcov on a source file that is older than one that is rebuilt by the test thing downstream causes problems. |
|
I would prefer the changes to the build order become a separate PR, built on top of this one. |
|
Let's get this in. Thanks @mattip and the reviewers @eric-wieser , @mhvk , @tylerjereddy. |
This s a first look at how the implementation of "NEP 0015" will look.Only one test pattern needed adjustment.There is now
multiarray.pyandumath.py, and a c-extension_multiarraycomprising the code from the two previous c-extension modules.The next step is to clean up the module initialization to avoid
set_numeric_opsEdit: s/test/test pattern/
Edit: strike out first sentence