Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove PyCXX dependency for core extension modules #3646

Merged
merged 18 commits into from Oct 18, 2014

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Oct 14, 2014

I realize that this PR is going to be difficult to review because of its size, but unfortunately, there was no good way to do this in pieces, given how interdependent may of our C++ extensions are. Rather than reading it all through, it would be helpful to just download, compile and run it in as many contexts as possible. Note that it's virtually net even in terms of lines of code ;)

In short, this removes the dependency on PyCXX, and uses the direct Python/C API instead (except for _tri). It is tested and working on Fedora 20, Ubuntu 14.4, Mac OS-X Mavericks. I need some help with Windows testing, particularly since this pushes the limits of the compiler in some places (@cgohlke?).

I skipped the _tri module is in part because it's not part of the tangled mess of most of the other extensions, but also because (I think) it's deprecated in favor of qhull (which already doesn't use PyCXX). But, I must admit, I haven't been following the world of triangulation very closely, so maybe we still need to keep _tri around for the foreseeable future. Cc: @ianthomas23

Most of these modules are declared private, so I took the liberty to change APIs when keeping them as-is would have caused a great deal of pain, but in general, this isn't a wholesale "rewrite" of anything, and really is meant to be mostly a refactor. The public Python API is unchanged, unless by accident.

An important side benefit of this work is to decoupling the extensions. For instance, in master, the _image module passes Image objects to the _backend_agg module for drawing (this also happens for fonts and text paths). Now, instead, Numpy arrays are used. This not only decouples the extensions from one another, it means that the data passed back and forth is a Numpy array and thus can be manipulated from Python much easier (Numpy arrays have a gazillion methods, which our dumb image buffers did not) and with fewer copies in some cases. So a lot of the methods here like rgba_to_argb etc. should become np.roll etc. There's still more work to be done there, and I think that in the long term these sorts of classes should completely disappear. As it stands, there are already some real benefits, such as saving "upside down" images to a PDF/SVG no longer requires a copy. Another nice side effect is that the "flipud_out" method, which statefully flips the stride orientation of an Image is now gone, since it's much easier/better to do that non-statefully using a Numpy array view. I plan to refactor this further when I tackle image resampling.

The following methods are gone, since they are just debugging tools, and now that we use Numpy arrays for buffers, it's much easier to poke and prod at the data as Numpy arrays:

  • FT2Font::attach_file
  • FT2Font::write_bitmap
  • RendererAgg::write_rgba
  • Image.fromarray2

Thanks to Mark Weibe for his numpy_cpp project on which some of this is based. He agrees that given its small size and matplotlib's somewhat unique needs, it was best to just fork it and include it in matplotlib, so that's what I've done. At this point, it bears only a subtle resemblance to the original.

@mdboom mdboom self-assigned this Oct 14, 2014
@tacaswell tacaswell added this to the v1.5.x milestone Oct 14, 2014
@efiring
Copy link
Member

efiring commented Oct 14, 2014

Wow! Thank you for executing this massive task!

@cgohlke
Copy link
Contributor

cgohlke commented Oct 14, 2014

Got it to compile (not tested) on Windows with msvc9 and msvc10 with the following changes:

diff --git a/src/ft2font.cpp b/src/ft2font.cpp
index bc242fd..6e7e0a2 100644
--- a/src/ft2font.cpp
+++ b/src/ft2font.cpp
@@ -622,7 +622,7 @@ int FT2Font::get_kerning(int left, int right, int mode)
 void FT2Font::set_text(
     size_t N, uint32_t *codepoints, double angle, FT_UInt32 flags, std::vector<double> &xys)
 {
-    angle = angle / 360.0 * 2 * M_PI;
+    angle = angle / 360.0 * 2 * 3.14159265358979323846;

     // this computes width and height in subpixels so we have to divide by 64
     matrix.xx = (FT_Fixed)(cos(angle) * 0x10000L);
diff --git a/src/mplutils.h b/src/mplutils.h
index a0160e0..ab6e7f3 100644
--- a/src/mplutils.h
+++ b/src/mplutils.h
@@ -63,4 +63,10 @@ inline T min(const T &a, const T &b)

 #endif

+#ifdef _MSC_VER && _MSC_VER <= 1600
+typedef unsigned __int8   uint8_t;
+#else
+#include <stdint.h>
+#endif
+
 #endif
diff --git a/src/numpy_cpp.h b/src/numpy_cpp.h
index dc5b25f..cf91767 100644
--- a/src/numpy_cpp.h
+++ b/src/numpy_cpp.h
@@ -116,6 +116,7 @@ struct type_num_of<npy_double>
         value = NPY_DOUBLE
     };
 };
+#if NPY_LONGDOUBLE != NPY_DOUBLE
 template <>
 struct type_num_of<npy_longdouble>
 {
@@ -123,6 +124,7 @@ struct type_num_of<npy_longdouble>
         value = NPY_LONGDOUBLE
     };
 };
+#endif
 template <>
 struct type_num_of<npy_cfloat>
 {
@@ -158,6 +160,7 @@ struct type_num_of<npy_clongdouble>
         value = NPY_CLONGDOUBLE
     };
 };
+#if NPY_CLONGDOUBLE != NPY_CDOUBLE
 template <>
 struct type_num_of<std::complex<npy_longdouble> >
 {
@@ -165,6 +168,7 @@ struct type_num_of<std::complex<npy_longdouble> >
         value = NPY_CLONGDOUBLE
     };
 };
+#endif
 template <>
 struct type_num_of<PyObject *>
 {

_gtkagg.cpp fails to compile:

src/_gtkagg.cpp(65) : error C2440: 'type cast' : cannot convert from 'numpy::array_view<T,ND>' to 'agg::int8u *'
        with
        [
            T=agg::int8u,
            ND=3
        ]
        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

src/_gtkagg.cpp(84) : error C2440: 'type cast' : cannot convert from 'numpy::array_view<T,ND>' to 'agg::int8u *'
        with
        [
            T=agg::int8u,
            ND=3
        ]
        No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

@ianthomas23
Copy link
Member

@mdboom: _tri is here to stay! delaunay, not _tri, is deprecated in favour of qhull. _tri contains the code for contouring of triangular grids and finding which triangles contain particular points; I expect it to hang around for a long time!

It should not be too difficult to switch _tri to use the Python/C API. Feel free to do it yourself and then you will have the pleasure of removing the CXX source code from mpl, or I can do it myself after this PR is committed.

@jenshnielsen
Copy link
Member

Looks great!

I gave it a quick test on my machine. The code compiles and installs but unfortunately it segfaults during the test suite at more or less random times. I haven't had time to dig more into this yet.

This is on OS X mavericks with Xcode 6.0 (clang 3.5 svn)

@mdboom
Copy link
Member Author

mdboom commented Oct 15, 2014

@ianthomas23: Ah, I was really hoping to get out of some work... No such luck. I'll try to tackle it.

@cgohlke: Thanks for the diff. I've incorporated the changes, with some minor changes to remain compatible with clang.

@jenshnielsen: I was testing with clang 3.4 -- I just upgraded and I'll let you know how I fare with clang 3.5.

@mdboom
Copy link
Member Author

mdboom commented Oct 15, 2014

@jenshnielsen: For what it's worth, the tests are passing for me on Mac Mavericks, XCode 6.0.1, using source-compiled python and the system Python. It's possible that this is just the result of some changes made earlier today to fix some segfaults on Travis. Haven't tried other pythons... If you can get a backtrace out of gdb for the segfault, that might point at something.

@jenshnielsen
Copy link
Member

Yes I will try to have a look. For the record I was testing with homebrew python build with homebrew tk needed for pymol. This seems to have some issues with the tk backend so it is possible that the issue is related to homebrew python.

@mdboom
Copy link
Member Author

mdboom commented Oct 17, 2014

This is rebased and has been updated for #3497.

@@ -3,174 +3,23 @@
/* A rewrite of _backend_agg using PyCXX to handle ref counting, etc..
Copy link
Member

Choose a reason for hiding this comment

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

Remove ref to PyCXX?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

@jenshnielsen
Copy link
Member

I can't reproduce the crash on Mac any more. I am seeing the same two tests fail as travis.

matplotlib.tests.test_quiver.test_quiver_animate.test

and

matplotlib.tests.test_png.test_pngsuite

@mdboom
Copy link
Member Author

mdboom commented Oct 17, 2014

I think I've got matplotlib.tests.test_png.test_pngsuite solved. I'm having trouble reproducing the test_quiver, but I see it on Travis, too.

@jenshnielsen
Copy link
Member

For the reference all the tests are passing for me locally too.

@mdboom
Copy link
Member Author

mdboom commented Oct 17, 2014

Ok! I'd say this is ready to merge from my end. Better to get this kind of thing merged early for lots of testing and to avoid merge conflicts...

@jenshnielsen
Copy link
Member

👍 Perhaps someone should test it a bit more with some interactive applications?

tacaswell added a commit that referenced this pull request Oct 18, 2014
MNT : Remove PyCXX dependency for core extension modules
@tacaswell tacaswell merged commit be34210 into matplotlib:master Oct 18, 2014
@tacaswell
Copy link
Member

I suspect we are going to spend the next few months tracking down strange seg-faults

:shipit:

@mdboom
Copy link
Member Author

mdboom commented Oct 20, 2014

Interesting tidbit -- the binary build is 12MB smaller now.

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.

None yet

6 participants