pyvista / fast-simplification

Fast Quadratic Mesh Simplification
https://pyvista.github.io/fast-simplification/
MIT License
108 stars 15 forks source link

Bug fixes and speed up of replay function #10

Closed Louis-Pujol closed 1 year ago

Louis-Pujol commented 1 year ago

Hello,

I discovered some bugs in the previous implementation of the replay features, and fixed it.

Bugs were :

In addition, I've optimized some parts of replay function where my previous implementation was too slow or memory inefficient. The interface didn't change.

It could be great if you can release this new version, I think it is now pretty stable. Perhaps some parts of replay function might be speeded up again, but for my use cases (< 1M vertices) I'm happy with the current running time.

Louis-Pujol commented 1 year ago

Hi, there was a problem for reading input arrays that were not C-contiguous (it was the case after a transpose for example).

I added a decorator to the functions simplify and replay to automatically call np.ascontiguousarray on non-contiguous arrays if needed.

I think the code is correct, the errror in the CI for building wheels on ubuntu seems to be related to Docker.

akaszynski commented 1 year ago

Looks like https://quay.io/pypa/manylinux2014_x86_64/ is down at the moment, see https://quay.io/ and https://status.quay.io/

akaszynski commented 1 year ago

Also, there are a handful of style fixes that should be resolved. Please see https://results.pre-commit.ci/run/github/434290916/1697214126.yTuVenK9REW8R9nk9A9q9g

Louis-Pujol commented 1 year ago

Thanks for the feedback. Now it seems that the line from pyvista import examples (used in test_replay.py) causes some trouble.

If I understand correctly the issue, we cannot import urllib3 because of a bad combination of openssl/python/urllib3 versions on the container that runs the tests (https://github.com/urllib3/urllib3/issues/2168). I tried to add sudo apt upgrade openssl to testing_and_deployment.yml but the issue remained the same. There's maybe a solution but I'm not skilled enough with CI to find a quick fix.

A simple workaround that made things work again was to skip the test that uses examples if from pyvista import examples raises an error.

Louis-Pujol commented 1 year ago

Thanks. I'm done with these changes, you can release this new version.