search

pyvista / fast-simplification

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

Trivial fix to trigger build #16

Closed akaszynski closed 10 months ago

akaszynski commented 1 year ago

Removes an unnecessary file to trigger the build. Seems to be failing on release/0.1.

akaszynski commented 1 year ago

@Louis-Pujol, running into a strange error where windows isn't simplifying. When you get a chance, would you mind looking into this? Both Linux and Mac work out, so it might have to do with some quirk with Windows (potentially int32 vs int64).

Louis-Pujol commented 10 months ago

Hi @akaszynski . Sorry for the delay, I completely missed the notification.

I have no clue about what's going on, I added the generation of pytest html reports to the CI pipeline. I'll try to detect the problem.

Louis-Pujol commented 10 months ago

Just added a workflow file that tests on ubuntu and windows and upload the reports from pytest. Every prints before failing inside tests function are reported there.

First observation : the test simplify_agg fails on windows, it does not simplify at all. However, the function simplify is working for other tests (I did some prints in test replay_sphere). Maybe it comes from the aggresivness arg. But I don't understand why outputs are different between ubuntu and windows. I'll have a look later, does anybody have an idea of where the issue comes from ?

Ubuntu:

ubuntu

Windows: windows

akaszynski commented 10 months ago

Biggest issue I've encountered between Windows and Linux when working with C has been the treatment of int vs. long as Windows and Linux treat the lengths of those int types differently. Not sure if it's an issue here, but it's best to use size_t or int32_t as much as possible.

akaszynski commented 10 months ago

Ran this locally on Windows with zero issues:

================================================= test session starts =================================================
platform win32 -- Python 3.11.3, pytest-7.4.3, pluggy-1.2.0
rootdir: C:\Users\Alex\source\fast-simplification
configfile: pytest.ini
collected 10 items

tests\test_map_isolated_points.py .                                                                              [ 10%]
tests\test_replay.py ....                                                                                        [ 50%]
tests\test_simplify.py .....                                                                                     [100%]

================================================= 10 passed in 4.40s ==================================================
PS C:\Users\Alex\source\fast-simplification>

Interestingly, this issue only arose when GitHub upgraded their Windows runner. I'm going to open another PR to use windows-2019.

Louis-Pujol commented 10 months ago

I investigated deeper the problem. Seems that the difference between ubuntu/windows is related to floating point computations. Here : https://github.com/Louis-Pujol/fast-simplification/actions/runs/7390279887 you can see it by looking at the log files.

I just run the test that fails on ubuntu and windows, it appears that the min error for ubuntu is 5.7e-8 and for windows 1.2e-7. As simplify is merging faces that have an error lower than 1.02e-7 with agg=1, no simplification occurred for windows.

My opinion is that the test faces.shape[0] < triangles.shape[0] should be changed to faces.shape[0] <= triangles.shape[0] if simplify is called with agg=1. It is possible that the number of faces differs from the number of triangles if all errors remain below the threshold. agg=1 is a very small value, the documentation of simplify() indicates that 5..8 are good values.

Another option is too keep faces.shape[0] < triangles.shape[0] but set agg=5 as a minimal reasonable value. In this case, the test with strict inequality pass.

Anyway this story of different value is interesting to examine, I'm curious to see if results will remain different across OS with windows-2019

Louis-Pujol commented 10 months ago

Update : with windows-2019 the threshold values are exactly the same than with ubuntu. https://github.com/Louis-Pujol/fast-simplification/actions/runs/7390480735

Sounds very weird that windows-latest leads to different values...

akaszynski commented 10 months ago

We'll build with Windows-2019 and hopefully the problem will fix itself in a few years when they drop 2019. Otherwise, we'll revisit it.