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

Python 3 support for model prediction (+ misc) #98

Merged
merged 4 commits into from
Jan 5, 2018

Conversation

gustavla
Copy link
Contributor

@gustavla gustavla commented Jan 5, 2018

This gets me passed all non-slow unit tests (with lots skipped - libsvm/xgboost). I will update with comments when I am able to run more tests and confirm they also work.

This PR does not include build configuration for Python 3.

#define PyBytes_Check(name) PyString_Check(name)
#define PyAnyInteger_Check(name) (PyLong_Check(name) || PyInt_Check(name))
#else
#define PyAnyInteger_Check(name) (PyLong_Check(name) || (_import_array(), PyArray_IsScalar(name, Integer)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is probably a better location where I can place _import_array(), but I wasn't sure of an appropriate entry point. This should be very fast after the first import though.

@@ -4,6 +4,15 @@
#include <pybind11/eval.h>
#include <pybind11/numpy.h>

#include <numpy/arrayobject.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be moved into the PY_MAJOR_VERSION=3, since numpy is not needed in Python 2. I will push a commit that moves this in.

@@ -21,6 +21,13 @@
import scipy.sparse as _sp


def to_unicode(x):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be _to_unicode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll update!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gustavla
Copy link
Contributor Author

gustavla commented Jan 5, 2018

I have now tested this with libsvm, xgboost, and keras, and as far as I can tell only tests that were already failing are failing. I'm merging this now.

@gustavla gustavla merged commit 3de3b5f into apple:master Jan 5, 2018
@gustavla
Copy link
Contributor Author

gustavla commented Jan 5, 2018

I don't know why github wanted two commits to resolve the merge conflict, so sorry for cluttering the history a bit!

Birch-san pushed a commit to Birch-san/coremltools that referenced this pull request Nov 27, 2022
* added kwargs for easier intialisation of random model

* initial commit for conversion script

* current debug script

* update

* Update

* done

* add updated debug conversion script

* style

* clean conversion script
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.

3 participants