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.

Sign up for GitHub

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

Jump to bottom

added first pass at python features #43

Closed
wants to merge 1 commit into from
Closed

added first pass at python features #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 76 additions & 18 deletions efel/cppcore/cppcore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,28 @@
#include <cfeature.h>
#include <efel.h>

#include <map>

#if PY_MAJOR_VERSION >= 3
#define IS_PY3K
#endif


extern cFeature* pFeature;

// Map of name -> feature
typedef std::map<string, PyObject *> PyName2feature;
static PyName2feature PyFeaturesMap;

static PyObject*
_getPyFunctionByName(string feature_name){
PyName2feature::iterator py_feature = PyFeaturesMap.find(feature_name);
if(py_feature != PyFeaturesMap.end()){
return py_feature->second;
}
return NULL;
}

static PyObject* CppCoreInitialize(PyObject* self, PyObject* args) {

char* depfilename, *outfilename;
Expand Down Expand Up @@ -112,7 +127,13 @@ static void PyList_from_vectorstring(vector<string> input, PyObject* output) {
}
}

static PyObject*
static PyObject*
_call_python(string feature_name, PyObject *py_function, PyObject* output){
PyObject* result = PyObject_CallFunctionObjArgs(py_function, output, NULL);
return result;
}

static PyObject*
_getfeature(PyObject* self, PyObject* args, const string &type) {
char* feature_name;
PyObject* py_values;
Expand All @@ -122,24 +143,29 @@ _getfeature(PyObject* self, PyObject* args, const string &type) {
return NULL;
}

string feature_type = pFeature->featuretype(string(feature_name));

if (!type.empty() && feature_type != type){
PyErr_SetString(PyExc_TypeError, "Feature type does not match");
return NULL;
}

if (feature_type == "int") {
vector<int> values;
return_value = pFeature->getFeatureInt(string(feature_name), values);
PyList_from_vectorint(values, py_values);
} else if (feature_type == "double") {
vector<double> values;
return_value = pFeature->getFeatureDouble(string(feature_name), values);
PyList_from_vectordouble(values, py_values);
PyObject* py_feature = _getPyFunctionByName(feature_name);
if (py_feature){
return _call_python(feature_name, py_feature, py_values);
} else {
PyErr_SetString(PyExc_TypeError, "Unknown feature name");
return NULL;
string feature_type = pFeature->featuretype(string(feature_name));

if (!type.empty() && feature_type != type){
PyErr_SetString(PyExc_TypeError, "Feature type does not match");
return NULL;
}

if (feature_type == "int") {
vector<int> values;
return_value = pFeature->getFeatureInt(string(feature_name), values);
PyList_from_vectorint(values, py_values);
} else if (feature_type == "double") {
vector<double> values;
return_value = pFeature->getFeatureDouble(string(feature_name), values);
PyList_from_vectordouble(values, py_values);
} else {
PyErr_SetString(PyExc_TypeError, "Unknown feature name");
return NULL;
}
}

return Py_BuildValue("i", return_value);
Expand Down Expand Up @@ -233,6 +259,34 @@ static PyObject* getgerrorstr(PyObject* self, PyObject* args) {
return Py_BuildValue("s", pFeature->getGError().c_str());
}

static PyObject*
registerFeature(PyObject* self, PyObject* args) {
char *name = NULL;
PyObject *function = NULL;
PyObject *result = NULL;

if (!PyArg_ParseTuple(args, "sO", &name, &function)) {
return NULL;
}
string feature_name = string(name);

if (!PyCallable_Check(function)) {
PyErr_SetString(PyExc_TypeError, "parameter must be callable");
return NULL;
}

PyObject* py_feature = _getPyFunctionByName(feature_name);
if (py_feature){
Py_DECREF(py_feature);
}

Py_INCREF(function);
PyFeaturesMap[feature_name] = function;

Py_INCREF(Py_None);
return Py_None;
}

static PyMethodDef CppCoreMethods[] = {
{"Initialize", CppCoreInitialize, METH_VARARGS,
"Initialise CppCore."},
Expand All @@ -258,6 +312,10 @@ static PyMethodDef CppCoreMethods[] = {

{"getDistance", getDistance, METH_VARARGS,
"Get the distance between a feature and experimental data"},

{"registerFeature", registerFeature, METH_VARARGS,
"Add a python function as an efeature"},

{NULL, NULL, 0, NULL} /* Sentinel */
};

Expand Down
19 changes: 19 additions & 0 deletions efel/tests/test_cppcore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,22 @@ def test_getFeature_non_existant(self):
"""cppcore: Testing failure exit code in getFeature"""
import efel.cppcore
efel.cppcore.getFeature("does_not_exist", list())

def test_registerFeature(self):
import efel.cppcore

def getTwiceAP_Amp(output):
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 is how to use it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that ideally the feature should use setFeature to store it's own values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's mainly used as an internal cache, correct?
I could keep that local to cppcore.cpp, if that makes sense. What is the invalidation policy for the cache at the moment (so I can mirror it)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure, but I think that cppcore.Initialize resets the cache ?

feature_values = list()
efel.cppcore.getFeature('AP_amplitude', feature_values)
output.extend(2*x for x in feature_values)
return len(feature_values)

self.setup_data()
efel.cppcore.registerFeature('getTwiceAP_Amp', getTwiceAP_Amp)
Copy link
Contributor

Choose a reason for hiding this comment

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

We would also need some mechanism to let different versions of the same feature live next to each other. Maybe we can add a version_string argument ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about just using the name of the feature do it: 'AP_amplitude_v1', 'AP_amplitude_v2', etc. That way, the calling convention doesn't have to change. Otherwise, getFeature has to take a version as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, that won't be general enough. The idea is that the user should be able to replace e.g. AP_amplitude with it's own version, 'and' that then every feature that depends on AP_amplitude will pick up the new implementation automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok.

Then, the idea would be that features can still depend on the old version if that's what their dependency says? That means we have to version all features, including the ones that already exist, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but that's why we have the dependency file and the liibv* versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, AP_amplitude wouldn't be overwritten for those, just for future ones?

features = list()
ret = efel.cppcore.getFeature('getTwiceAP_Amp', features)
nt.ok_(isinstance(features[0], float))
nt.eq_(5, len(features))
nt.ok_(np.allclose(2 * np.array([80.45724099440199, 80.46320199354948, 80.73300299176428,
80.9965359926715, 81.87292599493423]),
features))