search

mljs / tree-similarity

Tree similarity in Javascript
MIT License
8 stars 1 forks source link

refactor: separate createTree and getSimilarity #2

Closed targos closed 9 years ago

targos commented 9 years ago

do not merge. need to add more tests to make sure it works as expected.

targos commented 9 years ago

@lpatiny do you have a real testcase with NMR spectra that I could add ?

lpatiny commented 9 years ago

I will create a view that allows to generate spectra

targos commented 9 years ago

Just a thought: caching the tree in the spectrum may be convenient but we need to clearly document that if one wants to test different options, createTree has to be used

targos commented 9 years ago

@maasencioh Refactoring is over. I added a very simple testcase with two symmetric peaks and it seems that the tree creation is wrong. Can you take a look ?

maasencioh commented 9 years ago

I'm checking that

targos commented 9 years ago

@maasencioh @andcastillo can you take a look today ?

andcastillo commented 9 years ago

OK. We are together today.

targos commented 9 years ago

@maasencioh I just fixed the createTree function for the simple test case. Now the similarity tests don't pass. Did you get the values using the Java code or it was with your implementation ?

@andcastillo for the tree creation, I went with the usual approach in js for the from-to range (from is inclusive and to is exclusive). Does it match your implementation?

maasencioh commented 9 years ago

I don't remember but I'll check it now

andcastillo commented 9 years ago

@targos, yes, it has to be inclusive in one side, exclusive in the other side.

targos commented 9 years ago

@andcastillo I have made an example here with the current code: http://www.cheminfo.org/?viewURL=http%3A%2F%2Fcouch.cheminfo.org%2Fcheminfo%2F766371c949ba91421b91915a7b5b691f%2Fview.json&loadversion=true&fillsearch=TreeSimilarity

Is the generated tree correct in this case ?

andcastillo commented 9 years ago

@targos, It is giving the same result as in java. I think that the default threshold would be better defined as a percentage of the total integral. I had that absolute value in java, because all my spectra was normalised before going in the function. It was a trick. For example in the example you show, threshold = 0.002(0.2%), will give a tree that looks visually better:
screen shot 2015-09-28 at 11 08 08 am