Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

colorbraiding gives the wrong particles in error message #82

Closed
jeanluct opened this issue Nov 27, 2014 · 19 comments
Closed

colorbraiding gives the wrong particles in error message #82

jeanluct opened this issue Nov 27, 2014 · 19 comments
Assignees
Labels
Milestone

Comments

@jeanluct
Copy link
Owner

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

@jeanluct jeanluct added the new label Nov 27, 2014
@jeanluct
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

jeanluct commented Dec 7, 2014

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

@jeanluct jeanluct added this to the release-2.2 milestone Dec 7, 2014
@jeanluct jeanluct added bug and removed new labels Dec 7, 2014
@mbudisic
Copy link
Collaborator

mbudisic commented Dec 8, 2014

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
Copy link
Owner Author

jeanluct commented Dec 8, 2014

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
Copy link
Owner Author

jeanluct commented Dec 8, 2014

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.

@jeanluct jeanluct changed the title Can't create braid with one of Margaux's dataset? colorbraiding gives the wrong particles in error message Dec 8, 2014
@mbudisic
Copy link
Collaborator

mbudisic commented Dec 8, 2014

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
Copy link
Collaborator

mbudisic commented Dec 8, 2014

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
Copy link
Owner Author

jeanluct commented Dec 8, 2014

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
Copy link
Collaborator

mbudisic commented Dec 8, 2014

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

@jeanluct
Copy link
Owner Author

jeanluct commented Dec 8, 2014

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
#82 (comment).

@mbudisic
Copy link
Collaborator

mbudisic commented Dec 9, 2014

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
Copy link
Collaborator

mbudisic commented Dec 9, 2014

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

@jeanluct
Copy link
Owner Author

jeanluct commented Dec 9, 2014

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.)

mbudisic added a commit that referenced this issue Dec 9, 2014
Both crossingsToGenerators.m and crossingstogenerators_helper.hpp now return
sorted indices (colors) of coincident particles in error message,
which is then decoded by colorbraiding.m to match the indices of "slices" of XY
that are the cause of the error.
@mbudisic mbudisic closed this as completed Dec 9, 2014
@jeanluct
Copy link
Owner Author

jeanluct commented Dec 9, 2014

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

@jeanluct jeanluct reopened this Dec 9, 2014
@mbudisic
Copy link
Collaborator

mbudisic commented Dec 9, 2014

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
Copy link
Collaborator

mbudisic commented Dec 9, 2014

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

@jeanluct
Copy link
Owner Author

jeanluct commented Dec 9, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants