Skip to content

Python x64 and SCons Windows build support.#37

Closed
classner wants to merge 14 commits into
ndarray:masterfrom
classner:master
Closed

Python x64 and SCons Windows build support.#37
classner wants to merge 14 commits into
ndarray:masterfrom
classner:master

Conversation

@classner
Copy link
Copy Markdown
Contributor

@classner classner commented Oct 9, 2014

Dear Boost.NumPy team,

we are using Boost.NumPy and the ndarray project in a big data context at our lab. So I made the effort and made both projects x64 Python compatible, as well as the ndarrays 64bit indexed (similar to eigen3). I was mainly working on Windows and had to test the projects, so at the same time I extended the SCons build system to work with Windows.

After all changes, both projects build without any internal warnings (the numpy interface deprecation warning left aside) and are working flawlessly. I'll send pull requests for both, starting with Boost.NumPy.

Here, there wasn't much to do concerning x64 compatibility. The only related change is in boost/numpy/ndarray.hpp: the shape and stride of x64 numpy arrays are larger than int, and this can be adressed in general by using size_t.

The rest of the changes concerns building on Windows with SCons. I integrated the SConsChecks project (https://github.com/ChrislS/SConsChecks) and greatly simplified the SConscript. Apart from that, extension finding was simplified over tests and library files.

Best,
Christoph

@TallJimbo
Copy link
Copy Markdown
Member

Thanks for the contributions! These are fairly big code changes, so it make take me a little while, but I certainly do plan to incorporate your changes. I'll be posting review comments here and on the ndarray PR over the next week or so.

@nbecker, would you be up for taking a look at this and the corresponding ndarray/ndarray#37 as well? I'm particularly interested in finding out if anything here will cause any backwards compatibility problems for you. I hope not, as I think these changes actually include some features you requested a while back.

Comment thread boost/numpy/ndarray.hpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the typedef we want to switch to here is Python's Py_ssize_t, not std::size_t. I'm pretty sure the former is actually a signed type, but that's actually what we want, at least for strides (which can be negative). And I think it makes sense to make the shape signed as well, to avoid having to constantly work around compiler warnings about signed/unsigned comparisons.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right! I did a little bit of research on the matter and found http://legacy.python.org/dev/peps/pep-0353/. Quoting from there: "A new type Py_ssize_t is introduced, which has the same size as the compiler's size_t type, but is signed. It will be a typedef for ssize_t where available." It is additionally noted that "All occurrences of index and length parameters and results are changed to use Py_ssize_t".

The reasoning behind this is that negative indexing to indicate counting from the end would otherwise require significant code overhead. As a sidenote: I had to make a similar decision for the ndarray project. I decided to use std::size_t for array shapes and strides, but std::ptrdiff_t for iterator strides (making backwards iteration possible). This seemed most reasonable for me, but is of course debatable as well.

I do not have any experience with changing a pull request. Should I update the line in my branch and resend the request?

Christoph Lassner and others added 4 commits October 17, 2014 15:26
@TallJimbo
Copy link
Copy Markdown
Member

Sorry I've let this linger so long. I'm going to try to get it merged today, after changing all the sizes in Boost.NumPy to Py_ssize_t - after thinking about it some more, since that's what NumPy uses internally, we'd just be fooling ourselves if we thought switching to std::size_t here made things actually work better when converting STL containers to NumPy arrays, since it would just delay the conversion to when we call the NumPy C-API routines.

@TallJimbo
Copy link
Copy Markdown
Member

Looking in detail at the build system, it seems the command-line options for where to find Boost are broken. In particular, there are still a number of command-line options added in the main SConscript file of Boost.NumPy, as well as what seem to be a conflicting set of options in SConsChecks/boost.py. And I had expected --with-boost=$BOOST_DIR to be equivalent to --with-boost-include=$BOOST_DIR/include and --with-boost-lib=$BOOST_DIR/lib, but that seems no longer to be the case (the former did not work for me, but the latter did). Could you investigate, and either give me a hint as to how to fix this, or fix it yourself? I don't mind if you want to add a new set of options for specifying how to find boost, but I'd prefer for the old ones to continue to work unless that's architecturally impossible, and any new ones should ideally appear in scons --help.

@classner
Copy link
Copy Markdown
Contributor Author

I investigated and resolved the issue. It originated from Windows compatibility, since there the default library path is the 'stage\lib' folder instead of the 'lib' folder.

I updated the help in the SConscript file accordingly and modified the behavior to be as intended. I did not change any of the build options to maintain perfect backward compatibility of the project.

@classner
Copy link
Copy Markdown
Contributor Author

Builds and tests passing on Windows and Linux.

@TallJimbo
Copy link
Copy Markdown
Member

I've finally merged the build system changes from this PR (after rebasing to separate these commits from the integer type changes). The integer type changes are up next.

@classner
Copy link
Copy Markdown
Contributor Author

That's great to hear (also for ndarray)!

@TallJimbo
Copy link
Copy Markdown
Member

Merged to master.

@TallJimbo TallJimbo closed this May 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants