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

Use numpy arrays instead of python lists #23

Closed
wants to merge 2 commits into from

Conversation

ChristosT
Copy link
Contributor

Swapping python lists for numpy arrays removes the need of copying the data.

@lava
Copy link
Owner

lava commented May 2, 2017

Hi, thanks for the PR, I've been hoping someone would pick up this task eventually :)

However, it seems like you're currently hard-coding arrays of type NPY_DOUBLE, which would probably break if e.g. a vector<int> gets plotted?

@ChristosT
Copy link
Contributor Author

@lava yes you are right I missed that one ! I thought that double was enough because you were also converting them with PyFloat_FromDouble(x.at(i))) but PyArray_SimpleNewFromData may still break !

What about a templated type selector like this : http://stackoverflow.com/a/35061296
What do you think ? Do you have a better suggestion on how to handle this ?

@lava
Copy link
Owner

lava commented May 2, 2017

Yes, that looks like a good solution. Also, to prevent people from seeing some long and weird template error, I'd suggest adding a default case which will internally create a vector of doubles from the input data if the type is not recognized by the selector.

@ChristosT ChristosT force-pushed the numpy-dev branch 3 times, most recently from 9b444c7 to c0d5ef9 Compare May 7, 2017 01:09
@ChristosT
Copy link
Contributor Author

This should do it

@lava
Copy link
Owner

lava commented May 11, 2017

It looks good, but there is one more issue - numpy includes <Python.h>, which requires setting up the include path consistently with the include of <python2.7/Python.h> in matplotlibcpp.h.

Switching to numpy seems like a big enough gain that I'll probably just switch to Python.h in the main header file (or feel free do update the pull request doing it yourself), but I'll probably wait a few days to see if I can think of a solution to avoid this.

@lava
Copy link
Owner

lava commented May 13, 2017

Merged, thanks for the contribution!

@lava lava closed this May 13, 2017
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

2 participants