Skip to content

Conversation

@nbecker
Copy link

No description provided.

@nbeckernbecker mentioned this pull request Apr 12, 2017
@nbecker
Copy link
Author

Should probably add a test, but I'm not familiar with the codebase

@JohanMabille
Copy link
Member

xtensor-python is built with cmake:

  • the CMakeLists.txt file at the root of the project contains, among other things, the lists oth the headers:
set(XTENSOR_PYTHON_HEADERS ${XTENSOR_PYTHON_INCLUDE_DIR}/xtensor-python/pyarray.hpp ${XTENSOR_PYTHON_INCLUDE_DIR}/xtensor-python/pybuffer_adaptor.hpp ${XTENSOR_PYTHON_INCLUDE_DIR}/xtensor-python/pycontainer.hpp ${XTENSOR_PYTHON_INCLUDE_DIR}/xtensor-python/pytensor.hpp ${XTENSOR_PYTHON_INCLUDE_DIR}/xtensor-python/pyvectorize.hpp ${XTENSOR_PYTHON_INCLUDE_DIR}/xtensor-python/xtensor_python_config.hpp )

Each time you add a header to the project, you must update this list.

  • test/CMakeLists.txt contains the instructions to build the pure C++ tests (no python involved). Among them, you have the lists of source files:
set(XTENSOR_PYTHON_TESTS main.cpp test_pyarray.cpp test_pytensor.cpp test_pyvectorize.cpp )

As for headers, every new cpp test file should be added to this list. Tests rely on gtest, having a look at test/test_pytensor.cpp should show you how to do your own test.

  • test/test_python contains the python tests, you can run them with py.test. You can add your C++ code to the main.cpp file, and then update test_pyarray.py to add python tests.

@JohanMabille
Copy link
Member

I don't think you need to add the file pytraits.hpp, numpy_traits is already provided by xtensor-python in the file pycontainer.hpp.

@nbecker
Copy link
Author

If we add a test for noconvert, we should expect a testcase that will raise an exception. I didn't see any example of this type of test in the current test directory.

@JohanMabille
Copy link
Member

JohanMabille commented Apr 12, 2017

Indeed, I think you could rely on numpy.testing.assert_raises; I think pybind11 throws a cast_error when pyobject_caster<XXX>::load fails, but not sure about that.

@wolfvwolfv mentioned this pull request Apr 24, 2017
@JohanMabille
Copy link
Member

Fixed in #87.

Sign up for freeto 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

@nbecker@JohanMabille