jeanluct / braidlab

Matlab package for analyzing data using braids
GNU General Public License v3.0
23 stars 9 forks source link

Problems with colorbraiding and taffy (MEX) #116

Closed jeanluct closed 9 years ago

jeanluct commented 9 years ago

There is a discrepancy between the guide and current results. In fact there are quite a few.

Error using cross2gen_helper
Attempting to apply crossings resulted in inconsistency. Concurrent block at time
100.5, 4 crossings between 1 and 5 cannot be resolved.

This is a bit cryptic. Nothing about the projection line, unlike the old error message (see guide).

>> taffy('4rods')
Error using cross2gen_helper
Attempting to apply crossings resulted in inconsistency. Concurrent block at time
50.75, 2 crossings between 0 and 2 cannot be resolved.

still gives the error message, even though there should now be a well-defined braid.

So it sounds like we've introduced a regression of some kind. This is important and must be fixed before 3.1.

We should also see if this was ok as far back as 3.0.1.

(Split the non-MEX part of this to #117.)

jeanluct commented 9 years ago

See 3909cf0. You can now set a property prop('BraidAbsTol',1e-10). This is queried in colorbraiding and passed to cross2gen.m. You just need to do the same for the C++ code and rewrite the trivial eqfuzzy. Unless you want to keep your old version of eqfuzzy around, but if we need it later we could always go back to it.

I was going to have eqfuzzy query that property, but that would make it slow, and also make it difficult to implement in C++.

mbudisic commented 9 years ago

No need for that. In fact, we should probably get rid of eqfuzzy if we are going to stick with absolute tolerance as it makes the code less readable for such a simple operation.

In any case, there is a new problem right now that fails on taffy but I don't have time to fix it before heading out.

jeanluct commented 9 years ago

See commit 65cf259 (typo in issue number), as well as b89b285, 02783b2.

jeanluct commented 9 years ago

I don't think it's a new problem: it's the second one mentioned at the start of this issue. I think there were two problems, one with eqfuzzy but the other seems to be that the C++ code doesn't like simultaneous crossings somehow. You can try to bisect but I'm pretty sure that the parallel version of taffy('4rods') has never actually worked.

mbudisic commented 9 years ago

taffyTest fails sometimes, literally sometimes. Out of 5 repeated runs of run(taffyTest) , 4 times it passed, 1 time it failed because these two braids are not lexically equal, even though they are equal to each other:

    Actual Object:
         < 2  1  2  5  4  5  3  3  2  1  2  4  5  4 >
    Expected Object:
         < 2  1  2  4  5  4  3  3  2  1  2  4  5  4 >

I am guessing the issue is that a concurrent pair of crossings C1, C2 can be resolved equally as "C1 - C2" braid, and "C2 - C1" braid. I think this has to do with multithreaded execution, as different pairs of strands may be processed on different cores. If this happens, then depending on what else is going on at the computer, one core might terminate a bit ahead, or after, the other, which would affect the order in which their results are processed by the part of the algorithm that "integrates" pairwise crossings into a braid.

I have not managed to get the test to fail in the manner above when BRAIDLAB_threads = 1, but I did manage to do it in multithreaded mode. (I accidentally filed this under #117 instead...)

jeanluct commented 9 years ago

What is ABSTOL_TIME? There's no fragility involved in having this parameter?

jeanluct commented 9 years ago

In any case, I changed the unit test to check for braid equality, and now it works fine. It would be nice if we could guarantee that the answer returned was always identical, but being the correct braid is more important.

I will close this and go to 3.1, but also open an issue (#118).