search

giotto-ai / pyflagser

Python bindings and API for the flagser C++ library (https://github.com/luetge/flagser).
Other
13 stars 15 forks source link

compiling flagser, flagser-count with MANY_VERTICES to fix overflows … #68

Closed flomlo closed 2 years ago

flomlo commented 3 years ago

see PR #66 ; everything on the code the same, just different git branch.

ulupo commented 3 years ago

@MonkeyBreaker just waiting for your final blessing on this!

MonkeyBreaker commented 3 years ago

The changes seems good, I see that the flag was also added to flagser_count version, I think it would be a good idea to also add a test for this one. flagser and flagser_count are not exactly the same thing, even if most of their backend is the same.

Do you think it's worth it @ulupo to add a test ? @flomlo would you be able to add a test also for flagser_count version ?

flomlo commented 3 years ago

Mhm, I'ld say that this is overtesting, as the bug arises both in flagser and flagser_count in the shared section of the code, that is, the flag complex generation. Unless there are some MAJOR changes in flagser it will test exactly the same thing.

(I really hate writing test code, as well)

MonkeyBreaker commented 3 years ago

You're right, looking again at the code, I cannot see where it could produce a different/unexpected behavior.

(^^, after loosing a lot of time on a different project because we didn't have regression test in place at the time, I lost a lot of hair on a bug that I thought was fixed. That's why I'm now like "add test, add test" ...)

Then, @ulupo all green for me.

ulupo commented 2 years ago

I forgot why this was not merged ages ago already. @MonkeyBreaker is everything still green on your side?

MonkeyBreaker commented 2 years ago

Let me just test if this fixes #65 for last message posted, 5 minutes :)

MonkeyBreaker commented 2 years ago

With this fix, I can run without any exception thrown ! Thank you again @flomlo @ulupo Ok for the merge for me