Cython for ascii files

[nxsofsys]

Hi, I've added cython ascii parser, which much faster then pure python version. Tests passed in py35, but there can be issues with other environmnents.

[coveralls]

Coverage decreased (-16.6%) to 83.445% when pulling 18e9793dbb944c3037f9a7ac74dde0d989700144 on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage decreased (-16.6%) to 83.445% when pulling 18e9793dbb944c3037f9a7ac74dde0d989700144 on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage decreased (-16.6%) to 83.445% when pulling 26ee465ea9935b6347e7b1ffbd8bb130b4592247 on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[nxsofsys]

Need some help with coverage =)

[wolph]

Nice, looks like a very nice fix :)

As for the test coverage, when you run py.test --cov-report html it should generate a html coverage report to help a bit. But I'll take a look.

[wolph]

Is there a specific reason that you renamed stl to src? Looks like that might break backwards compatibility

[nxsofsys]

Is there a specific reason that you renamed stl to src? Looks like that might break backwards compatibility

Yes, it's related to tests. Tests try import stl package from stl folder where 'stlascii' extension not exists yet, but they should import stl from package installed in environment. To avoid missing import I renamed stl to src, while package still installed in as stl. It can be changed if there is more correct resolution. You can rename it back and try to run tox =)

As for the test coverage, when you run py.test --cov-report html it should generate a html coverage report to help a bit. But I'll take a look.

I'll try =)

[wolph]

I've made a few small changes and a large portion appears to be working but some of the tests are broke due to the binary ascii version having different behaviour.

The branch: https://github.com/WoLpH/numpy-stl/tree/cython_ascii The test results: https://travis-ci.org/WoLpH/numpy-stl/builds/153546235

The most important difference appears to be the handling of the header and/or broken files.

[nxsofsys]

Hm, but my version passes all tests, including broken file header. I see in test results that install procedure failed - https://travis-ci.org/WoLpH/numpy-stl/jobs/153546244 Here https://github.com/WoLpH/numpy-stl/blob/cython_ascii/tests/requirements.txt cython included twice.

Moreover seems you forgot to read name of solid in https://github.com/WoLpH/numpy-stl/blob/cython_ascii/stl/_speedups.pyx =)))

I don't think that lowering every string is nesessary, because lowering may be needed in three cases when actually line count is much larger. Keywords like 'sOlid' or 'soliD' so rare and can be ommited i think =) At least comparing with 'Solid' can be added instead of lowering every line =) in extremly large files it does matter.

[wolph]

Oops, did I break it? Perhaps I did... I'll check again :)

[wolph]

You're right, I accidently removed the name copy code and included the requirements twice... for some reason it didn't install cython for me so I added it again. Must be user error, I'm a bit tired at the moment ;)

As for lowering every string, while it's not need in most cases the original code lowers the lines so it should behave the same to stay compatible. I believe the current version incurs very little performance overhead but please give it a try to see if it's fast enough :)

For extremely large files I would recommend using the binary format anyhow, it's much faster.

There is just 1 small problem remaining, when testing with more verbosity the code currently segfaults. Do you have any idea what's causing it?

[wolph]

Nevermind, I found it. It's because the line buffer is too small to store the entire buffer

[nxsofsys]

For extremely large files I would recommend using the binary format anyhow, it's much faster.

Sometimes binary format not acceptable, for example in OpenFOAM when solid names is required for correct calculations. Lowering strings not using in VTK for example, so if numpy-stl will load keyword like "sOlId" but VTK is not it will be strange =) Overhead, of course, is very little, but I prefer error on bad keyword instead of silent loading, because other tools will not load files like this and it may be confusing ))

[nxsofsys]

By including _speedups.c file in setup.py you broke windows build =)))

[wolph]

Hmm... including the .c file was recommended by the Cython docs. Apparently it doesn't work all that fool-proof :P

As for the case sensitivity thing... does the stl standard require a specific case? I'm personally ok either way as long as it's consistent with the pure-python version so there are no unexpected bugs.

Now I'm only wondering why the build doesn't function on travis... for some reason tox on travis doesn't use the compiled module but locally it does pass the tests perfectly.

[nxsofsys]

Now I'm only wondering why the build doesn't function on travis... for some reason tox on travis doesn't use the compiled module but locally it does pass the tests perfectly.

Thats why I renamed stl to src )) As I said, when tox is running it uses import from local stl folder, not from site-packages, because local folder with init.py file has higher priority over stl package installed in environment =) Unfortunately, latest pure python version fails multi file tests on windows cos pure python version does not restore file pointer correctly when CRLF used. It caused by writing file in w+ mode and reading in rb mode.

[wolph]

Hrm... that makes sense. I've encountered similar problems before but never actually looked for the cause. Thanks for the tip :)

I believe I've fixed the problems now but travis is still testing. Stay tuned :)

As for the windows issue, I'll try to get appveyor up and running to test

[wolph]

At least the travis builds are fully working. The appveyor builds are still a work-in-progress

[nxsofsys]

At least the travis builds are fully working

I added import error raising here

try:
    from . import _speedups
except ImportError:  # pragma: no cover
    _speedups = None
    raise # raise import error

Py.test and tox still showing error "ImportError: cannot import name '_speedups'" . Is it same for travis?

[wolph]

The travis test indicates 100% coverage so it has to cover the code somehow. Locally it works for both py.test and tox but I've added the raise to test

[wolph]

Ok, with the raise in place it seems that it's working for python 3.5 but not for the rest. Assumingly because it gets the package from the environment in that case.

I'll try to debug the travis environment a bit further. I'm beginning to wonder if this is worth it... especially since packaging will be a problem too. I'll need to switch from universal wheels to binary ones :(

[nxsofsys]

I'll need to switch from universal wheels to binary ones :(

It's not packaging problem. It caused by import from stl folder, look: http://prntscr.com/c83uh6 http://prntscr.com/c83v6s http://prntscr.com/c83wer http://prntscr.com/c83wo2

Package works correctly, you can find builded extension in site-packages, but as I said before folder stl in git repository conflicts with package name, and prevents python from importing package from site-packages. Just rename stl folder in repository to something else to reslove conflict or move test working directory away from repository root.

My version of package has no problem with install on windows or linux. We are using it over one month.

[wolph]

It's not packaging problem. It caused by import from stl folder, look:

You're confusing two separate issues. This isn't a packaging problem but there will be a packaging problem for the next release. I can't build windows or linux extensions locally (I'm on OS X) which means that I'll need to find a way to distribute windows and linux specific wheels as well. Or package without the speedups and requiring people to do a source install if they require it.

As for the problem itself, I am aware of the cause but I disagree with your solution. Renaming the directory is working around the problem instead of solving it. And that actually isn't the problem anymore since I've added the chdir to the tests directory yesterday. The problem is that the package won't build unless numpy and cython are installed which creates a bit of a chicken-and-egg problem. Although I believe I've fixed that too by setting tox to use develop instead of sdist now.

[wolph]

Can you give the latest build a try and see how/why it's not functioning on Windows?

It works perfectly on Linux and OS X: https://travis-ci.org/WoLpH/numpy-stl/builds/153780340

[nxsofsys]

Only one test failed now: http://prntscr.com/c8463t Packaging and importing seems OK now, great job =) I do not think that _speedups module is required everywhere, if cython not available just fallback to pure python version. This is done by python-msgpack for example. On Windows Visual Studio is required for building too, not only cython.

[nxsofsys]

And that actually isn't the problem anymore since I've added the chdir to the tests directory yesterday.

I disagree =) even you added this line test importing 'stl' from repo's stl folder, not from installed package =)) usedevelop = True in tox configuration forces extension to be created in repo's 'stl' folder, so tests runs fine, but it is not clear installation test since package imported not from python environment, but from repository folder. So lines "changedir = tests" and "skipsdist = True" can safely be removed now =)

[coveralls]

Coverage remained the same at 100.0% when pulling a394dca57c0d4ec5abf98455e277ff0a23a47b9b on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 4edca18aa711cd893ffcfd52187a6fb04703e747 on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 48756f7549ca62fd64bdfa31cc204633f209e068 on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling afb09482d6f8565859e3be12a930bd2dafa5cfb8 on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 49fb46c5ccf957f8577daec8c823f9e3469e410c on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[nxsofsys]

All checks have passed =)))

No =))) I broke appveyor again!)))

[coveralls]

Coverage remained the same at 100.0% when pulling bc8ad06320232ca304c8e1b1acf543ccdf4701bc on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling fb849215aee007c53bab4d458f893c8379c6f6cc on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 700e0195d932bee7712421e9030cfb27f99e5b8a on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 700e0195d932bee7712421e9030cfb27f99e5b8a on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 5d0f86bf37b442d38d95be50cdcf9cfea5c9b97e on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 5d0f86bf37b442d38d95be50cdcf9cfea5c9b97e on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 5d0f86bf37b442d38d95be50cdcf9cfea5c9b97e on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 5d0f86bf37b442d38d95be50cdcf9cfea5c9b97e on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[coveralls]

Coverage remained the same at 100.0% when pulling 5d0f86bf37b442d38d95be50cdcf9cfea5c9b97e on diceproject:cython_ascii into dd00e90b81cfe62c73a59f360e5c4c70a2391030 on WoLpH:develop.

[nxsofsys]

that was hard =))

[wolph]

Wow... looks like it indeed.

I disagree =) even you added this line test importing 'stl' from repo's stl folder, not from installed package =)) usedevelop = True in tox configuration forces extension to be created in repo's 'stl' folder, so tests runs fine, but it is not clear installation test since package imported not from python environment, but from repository folder. So lines "changedir = tests" and "skipsdist = True" can safely be removed now =)

You're right... it didn't function as I had hoped :(

Thanks for all the help, I'm merging and packaging a new version right now :)

[nxsofsys]

Thank you for amazing modules =) Do you wanna make another package for 3D OBJ files?)))

[wolph]

I can't say I need it myself but I'm open to it. I still have a C++ obj parser somewhere around here which would be easy enough to wrap :)

There might be a few good alternatives available though, have you tried PyWavefront or Trimesh? Especially Trimesh looks pretty good but I can't say I've tried the package. If it doesn't do the trick for you I'll create the package :)

[wolph]

I've deployed the new release btw, version 2.0.0 is out thanks to you :)

[nxsofsys]

Trimesh looks nice, but our ascii parser on cython is faster twice than tricky numpy parser in Trimesh =)

[wolph]

That's fair, I suppose I can work on a similar package for obj files. Any votes on naming the package? numpy-obj seems a bit too generic so numpy-wavefront might be better but perhaps not too clear...

Something such as numpy-3dobj could be a good one