ENH: implement nep 0015: merge multiarray and umath by mattip · Pull Request #10915 · numpy/numpy

mattip · 2018-04-16T12:03:16Z

~~This s a first look at how the implementation of "NEP 0015" will look.~~ Only one test pattern needed adjustment.

There is now multiarray.py and umath.py, and a c-extension _multiarray comprising the code from the two previous c-extension modules.

The next step is to clean up the module initialization to avoid set_numeric_ops

Edit: s/test/test pattern/

Edit: strike out first sentence

eric-wieser · 2018-04-17T05:16:58Z

Why are we trying to import a top-level module?

see comment below, maybe we don't need this method at all?

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.

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

eric-wieser · 2018-04-17T05:19:05Z

Given you already have the import * above, do you really need this? I'd be inclined to drop the import * and stick with this.

eric-wieser · 2018-04-17T05:20:10Z

Maybe rename to _multiarray_umath or something with both names, to make it clearer for people who already know about multiarray existing?

ok, adopted

mattip · 2018-04-17T06:33:10Z

about _import_array - AFAICT it has a few reasons for existing:

make sure multiarray is imported (useful internally before the modules are merged)
make sure _ARRAY_API is initialized, which is also checked when importing multiarray
perform runtime checks on byte order

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 _multiarray_umath initialization. So _import_array becomes an alias for third-party users to PyImport_ImportModule("numpy.core.multiarray") and we do not need to call it internally. It can also move out of generate_numpy_api.py to one of the numpy headers.

Am I missing something?

mattip · 2018-04-24T13:34:28Z

Still WIP. I did not adapt umathmodule.c for the c-api import as documented here so calling any c-api function from umathmodule crashes. I tried changing PY_ARRAY_UNIQUE_SYMBOL to NO_IMPORT_ARRAY but the initialization still crashes. Still investigating.

mhvk

So far, it looks far less intrusive than I would have thought! Some mostly ignorant queries...

mhvk · 2018-04-24T14:01:57Z

umath part!

copy-paste. Fixed

mhvk · 2018-04-24T14:06:06Z

These disappear because they are already included above, correct?

mhvk · 2018-04-24T14:10:15Z

I'm very confused, shouldn't it be _multiarray_umath here and below?

fixed, thanks

charris · 2018-04-24T15:11:22Z

Just to note that I want to branch 1.15 before this goes in.

mattip · 2018-04-24T15:22:55Z

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

mhvk · 2018-04-24T15:57:29Z

@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.

mattip · 2018-06-21T23:41:25Z

Rebased off master after 1.15

mhvk · 2018-06-22T01:35:03Z

I'm quite excited about this - do ping if this is ready for further review!

mattip · 2018-06-25T20:38:45Z

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 cblasfuncs.c routines from umath. That is why I added commits a191224 and b997efd.

Note that I only moved files that have not changed in quite a while and that have no NUMPY_API functions into common (used to be private) in order to minimize git churn on possible pull requests and issues. Eventually, we may wish to restructure the source code directories according to a different paradigm.

mhvk

Looks very good, and I'm excited about it! My comments are really two, both organisational: whether to have common_src in setup.py and a new umathmodule.h to keep things umath-related in the multiarray initialization.

mhvk · 2018-06-26T18:29:23Z

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?

mhvk · 2018-06-26T18:31:34Z

Might it be an idea to have a common_src here?

adopted. Eventually everything will become common

mhvk · 2018-06-26T18:34:51Z

Might it be more logical to have this contained in a separate umathmodule.h file?

It seems there may be unused includes, reworking

I cleaned this up by removing many uneeded include directives

mhvk · 2018-06-26T18:35:10Z

If following the separate umathmodule.h file idea above, these lines could go in there too.

cleanup up via removal and refactoring, without a umathmodule.h

on second thought umathmodule.h would be better

mhvk · 2018-06-26T18:37:49Z

Given the comment, should this be removed?

mhvk · 2018-06-27T15:25:37Z

From this one on down, these are duplications.

mattip · 2018-07-02T23:49:43Z

rebased off master to solve merge conflicts, added common/umathmodule.h

…Array_API)

…laces

mattip · 2018-08-21T17:09:15Z

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.

tylerjereddy · 2018-08-21T18:04:49Z

mem_overlap.gcda is still an issue here, but rebasing did resolve a few of the other timestamp issues; perhaps there are still components of the NumPy code base that drive redundant compilation beyond the changes in this PR then?

tylerjereddy · 2018-08-21T19:21:12Z

-                             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')],


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.

mattip · 2018-08-22T04:10:34Z

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.

tylerjereddy · 2018-08-22T05:12:06Z

I'm not sure I follow -- the setup.py itself is pretty confusing -- shouldn't there be clear comments explaining why the build order is the way it is, and why it is necessary to include the same source files in duplicate in this manner? Just reading through makes it seem magical or like an artifact of the way the source evolved over time.

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.

mattip · 2018-08-22T08:58:45Z

I would prefer the changes to the build order become a separate PR, built on top of this one.

mattip · 2018-08-31T12:23:52Z

This is waiting for another core reviewer, @mhvk suggested @charris, @pv, @njsmith

charris · 2018-08-31T16:12:28Z

Let's get this in. Thanks @mattip and the reviewers @eric-wieser , @mhvk , @tylerjereddy.

eric-wieser reviewed Apr 17, 2018

View reviewed changes

mattip force-pushed the implement-nep-0015 branch from 9cc4aca to 3703c72 Compare April 24, 2018 13:26

mhvk reviewed Apr 24, 2018

View reviewed changes

mattip force-pushed the implement-nep-0015 branch from 3703c72 to 00e3433 Compare April 24, 2018 14:17

mattip force-pushed the implement-nep-0015 branch 2 times, most recently from 88b21fe to 28ce0b4 Compare April 27, 2018 06:28

mhvk changed the title ~~WIP: ENH: implement nep 0015~~ WIP: ENH: implement nep 0015: merge multiarray and umath Jun 2, 2018

mhvk mentioned this pull request Jun 2, 2018

Ufunc calls on scalars are very slow #11232

Open

mattip mentioned this pull request Jun 13, 2018

DEP: Function set_numeric_ops #10141

Closed

mattip force-pushed the implement-nep-0015 branch from 28ce0b4 to 010fc4f Compare June 21, 2018 23:41

mattip mentioned this pull request Jun 25, 2018

NEP 15: Merging multiarray and umath #10704

Merged

mattip force-pushed the implement-nep-0015 branch from 010fc4f to b997efd Compare June 25, 2018 19:35

mattip changed the title ~~WIP: ENH: implement nep 0015: merge multiarray and umath~~ ENH: implement nep 0015: merge multiarray and umath Jun 25, 2018

mhvk reviewed Jun 26, 2018

View reviewed changes

mhvk reviewed Jun 27, 2018

View reviewed changes

mattip force-pushed the implement-nep-0015 branch 3 times, most recently from de16ebe to bb2cc21 Compare July 2, 2018 23:48

mattip force-pushed the implement-nep-0015 branch from bb2cc21 to 9246b07 Compare July 3, 2018 23:01

mattip added 12 commits August 21, 2018 20:03

MAINT: merge umath, multiarray into _multiarray, add python wrappers

31df302

MAINT: define _MULTIARRAYMODULE to use functions directly (not via Py…

e8aea2e

…Array_API)

MAINT: move declararions into umathmodule.h

f88d576

MAINT: fix more name replacements, __all__ imports

ad1f5b5

MAINT: move scalar math intialization to before setup_scalartypes

92f01dd

MAINT: move private -> common

bd4500f

MAINT: move unchanging, non-api files to common

1bca48b

MAINT: create common_{src,dep}, move header includes to appropriate p…

c49f779

…laces

DOC: write release note

c9914e4

MAINT: export newly added c-extension function

9592ee2

DOC: add docstring to new python files

8725087

MAINT: remove code that codecov pointed out is superflous

03e0814

mattip force-pushed the implement-nep-0015 branch from a6bf7e9 to 03e0814 Compare August 21, 2018 17:07

tylerjereddy reviewed Aug 21, 2018

View reviewed changes

tylerjereddy mentioned this pull request Aug 21, 2018

WIP, TST: include missing source files in gcov report #11791

Closed

charris merged commit f36d2d4 into numpy:master Aug 31, 2018

This was referenced Sep 1, 2018

BUG: Re-add _ones_like to numpy.core.umath. #11862

Merged

TST: AttributeError numpy.core.umath has no attribute _ones_like astropy/astropy#7791

Closed

rgommers mentioned this pull request Sep 4, 2018

BUG: ImportError: cannot import name 'ellipkm1' with numpy-dev scipy/scipy#9220

Closed

charris mentioned this pull request Sep 4, 2018

ImportError: numpy.core._multiarray_umath failed to import #11871

Closed

njsmith mentioned this pull request Dec 9, 2018

The epic dtype cleanup plan #2899

Closed

1 of 2 tasks

mattip deleted the implement-nep-0015 branch March 4, 2019 13:17

ajoubertza mentioned this pull request May 4, 2019

Required numpy >= 1.1 version is not enough? tango-controls/pytango#275

Closed

Conversation

mattip commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Apr 17, 2018

Uh oh!

mattip commented Apr 24, 2018

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Apr 24, 2018

Uh oh!

mattip commented Apr 24, 2018

Uh oh!

mhvk commented Apr 24, 2018

Uh oh!

mattip commented Jun 21, 2018

Uh oh!

mhvk commented Jun 22, 2018

Uh oh!

mattip commented Jun 25, 2018

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Apr 16, 2018 •

edited

Loading