jeanluct / braidlab

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

colorbraiding gives the wrong particles in error message #82

Closed jeanluct closed 9 years ago

jeanluct commented 9 years ago

Margaux has a dataset that braidlab can't handle, and I can't see why. I attach the dataset.

jeanluct commented 9 years ago

From Jean-Luc Thiffeault on 2014-11-16 19:40:13+00:00

Note that this is a partial dataset containing only the first two time points.

jeanluct commented 9 years ago

From Jean-Luc Thiffeault on 2014-11-16 20:00:10+00:00

Ok, I understand the issue: since colorbraiding needs to sort the particles from left to right according to initial position, it returns an error message mentioning the wrong particles. In this case it claims particles 1 and 2 are coincident, when it's really 1 and 3.

I fixed this in colobraiding.m for the Matlab method. Marko: please fix in the C++ code. The way I did it requires passing the idx array. This is kind of ugly but doesn't really affect overhead. Maybe there's a better way? It's annoying but I think it's important that the error message refers to the right particles.

jeanluct commented 9 years ago

From Jean-Luc Thiffeault on 2014-11-16 20:00:55+00:00

Pass idx so that colorbraiding gives the right error msg. See issue #82.

→ <<cset 6fe24fe6fd40>>

jeanluct commented 9 years ago

Marko: was this fixed in the C++? The error message returning the wrong permutation?

mbudisic commented 9 years ago

Is there a dataset that I can use to test for this error? I cannot find it in my e-mails... Does it make sense to make a unit test for this? I opened an iss82 branch for this issue.

jeanluct commented 9 years ago

I think it used to be attached to this issue but got lost in the migration from BitBucket. It's too bad that you can't attach things to a GitHub issue (except images).

Maybe I can just quote the values:

XY(:,:,1) =

   0.025000000000000  -1.025000000000000
   0.023723980025734  -1.024702440850582

XY(:,:,2) =

   0.025000000000000  -0.925000000000000
   0.025195202215646  -0.925081384217894

XY(:,:,3) =

   0.025000000000000  -1.025000000000000
   0.023723980025734  -1.024702440850582

XY(:,:,4) =

   0.025000000000000  -1.075000000000000
   0.023183108848995  -1.074611892145736

Without MEX, this generates the error

>> braid(XY)
Error using braidlab.braid/colorbraiding>crossingsToGenerators (line 186)
Particles 2 and 4 have coincident projection at time index 1: change projection
angle (type help braid.braid).

Error in braidlab.braid/colorbraiding (line 112)
    [gen,tcr,~] = crossingsToGenerators(XYtraj,t,idx);

Error in braidlab.braid (line 213)
        br = braidlab.braid.colorbraiding(b,1:size(b,1),secnd);

With MEX, we have

>> braid(XY)
Error using colorbraiding_helper
Particles 1 and 2 have coincident projection at time index 1: change projection
angle (type help braid.braid).
[Error 1/1 in threads.]

Error in braidlab.braid/colorbraiding (line 105)
  [gen,tcr] = colorbraiding_helper(XYtraj,t,Nthreads);

Error in braidlab.braid (line 213)
        br = braidlab.braid.colorbraiding(b,1:size(b,1),secnd);

So you see in the MEX version it gives the wrong particles.

If you change the projection angle, the error becomes coincident coordinates:

>> braid(XY,.1)
Error using colorbraiding_helper
Particles 2 and 3 have coincident coordinates at time index 1: braid not defined.
[Error 1/1 in threads.]

But it's 1 and 3 that have coincident coordinates.

jeanluct commented 9 years ago

Oh, and I don't think it's worth writing a unit test, since this is an error in the error message itself...

In fact it's esoteric enough that if it's going to take some work to fix, then we could kick it beyond 2.2. Your choice.

mbudisic commented 9 years ago

Here's the matrix with copy-paste supported.

XY = nan(2,2,4);
XY(:,:,1) = [0.025 -1.025;0.023723980025734 -1.02470244085058];
XY(:,:,2) = [0.025 -0.925;0.025195202215646 -0.925081384217894];
XY(:,:,3) = [0.025 -1.025;0.023723980025734 -1.02470244085058];
XY(:,:,4) = [0.025 -1.075;0.023183108848995 -1.07461189214574];
mbudisic commented 9 years ago

While I do understand what's going on (I think, at least), I get different errors:

global BRAIDLAB_braid_nomex; BRAIDLAB_braid_nomex = 1;
braidlab.braid(XY)

gives

Particles 4 and 1 have coincident projection at time index 1: change projection angle (type help braid.braid).

And it should be 1 and 3 right? Are different branches doing different things? Can you confirm that I understand things correctly?

global BRAIDLAB_braid_nomex; BRAIDLAB_braid_nomex = 0;
braidlab.braid(XY)

gives (incorrectly)

Particles 1 and 2 have coincident projection at time index 1: change projection angle (type help
braid.braid).
jeanluct commented 9 years ago

Maybe it has to do with re-reading the data: I loaded mine from a MEX file. Do you have access to the shared Dropbox folder with Margaux? It has a file called issue82.mat.

mbudisic commented 9 years ago

I don't, can you share it with me or e-mail me the file? It's small enough...

jeanluct commented 9 years ago

Not sure why I didn't just do that...

On Mon, Dec 8, 2014 at 3:32 PM, Marko Budisic notifications@github.com wrote:

I don't, can you share it with me or e-mail me the file? It's small enough...

— Reply to this email directly or view it on GitHub https://github.com/jeanluct/braidlab/issues/82#issuecomment-66191294.

mbudisic commented 9 years ago

OK, I loaded the issue82.mat but it still looks like particles 1 and 3 are culprits, not 2 and 4 as Matlab code reports. Could the reason be that it's not the permutation vector that we want to pass, but rather its inverse?

mbudisic commented 9 years ago

Added devel/tests/test_issue82.m to demonstrate this.

jeanluct commented 9 years ago

No I think it works now: I added a projection angle of .1. The errors were mixed between coincident projection and coordinates, but if you rotate the projection angle there is no projection error, only coincident particles. The numbers seem to match now. (Committed on the same branch.)

jeanluct commented 9 years ago

I merged develop into iss82 and the testsuite doesn't work. Please check.

mbudisic commented 9 years ago

testsuite works on my end... (and it did before the commit too) I just pulled, fetched and everything.

Weren't you supposed to merge iss82 into develop (and not vice versa)?

mbudisic commented 9 years ago

Aha, without MEX it fails. I'll take a look.

jeanluct commented 9 years ago

I merge from develop to iss82 to see if it would solve the testsuite problem. It seems all I needed was a make clean. Works now.

Will merge into develop.