fixed #152 #189
fixed #152 #189
Conversation
There was a problem hiding this comment.
Thanks @kwcantrell! This is a huge step in the right direction. I have a few questions / suggestions for things to take care of before merging (or at least before releasing an initial version), but these shouldn't be too dramatic to fix.
empress/support_files/js/empress.js
Outdated
| // because branches in this group may be elements of different | ||
| // groups. For example "b1" may belong to skin and oral while | ||
| // "b2" may belong to gut and skin. | ||
| for (var elem of result["non-unique"]) { |
There was a problem hiding this comment.
It looks like this will silently fail if one of the groups is literally named "non-unique". At the very least, we should throw an error / alert / etc. if this is the case (ideally we would store things in such a way that we could avoid this problem).
There was a problem hiding this comment.
We should probably throw an error if "non-unique" is an actual group name (which in this context are values in the metadata files). The reason being, whatever group names are in result, are the name that will be displayed in the legend. That or we will have to rework the way legend.js functions.
empress/support_files/js/empress.js
Outdated
| // up the tree? Current behavior does not project it up the tree | ||
| // because branches in this group may be elements of different | ||
| // groups. For example "b1" may belong to skin and oral while | ||
| // "b2" may belong to gut and skin. |
There was a problem hiding this comment.
This behavior seems reasonable to me, at least. 👍 on the detailed explanation for this
empress/support_files/js/empress.js
Outdated
| observationsPerGroup = this._projectObservations(observationsPerGroup); | ||
| observationsPerGroup = this._projectObservations( | ||
| observationsPerGroup, | ||
| false |
There was a problem hiding this comment.
I'm confused as to why addNonUnique is only sometimes true. Forgive me if I'm forgetting something, but I feel like this should always be true?
There was a problem hiding this comment.
If addNonUnique is set to true, _projectObservations() will add another group element to its return object which is the object used to color the tree. However, a lot of the coloring function created their coloring maps based on the groups that were passed into _projectObservations(). This resulted in multiple functions breaking because when they iterated through the groups in the return object from _projectObservations() to color the tree, they would usedthe group names to index into the color map and since "non-unique" was added after the color map was created, this resulted in multiple errors.
The two areas were addNonUnique is set to false are 1) feature based metadata coloring (which doesn't have non-unique branches) and 2) selecting nodes in emperor (which although can contain non-unique branches, it doesn't display a legend so, it wont be obvious to the user what the color representing non-unique branches means since the other colors will match emperor sample colors).
There was a problem hiding this comment.
Thanks for the explanation! That makes this a lot clearer.
I suspect we may want to develop things such that we don't need these special cases. We can discuss it at today's meeting, but I think one simple way to handle this is setting the (default) "non-unique" color to be the same as the (default) tree node color -- this would make it a lot clearer that non-unique internal nodes and non-unique tips are colored the same way because they both aren't "unique" to a category. I suppose this would then mean that "non-represented" tips/nodes might need to be colored a different "technical" color, to quote @ElDeveloper -- maybe, by default, non-unique nodes could be colored gray and non-represented nodes could be colored black? We can hash out the details together.
(Even though there aren't non-unique tips in feature metadata coloring, the internal nodes are (basically always) still going to be non-unique, anyway -- consistency here is important, IMO. And for the Emperor-selection highlighting, we should probably handle things consistently (and maybe show a legend anyway). Of course, making that pull up a legend is probably outside this PR's scope, so we can open an issue for it :)
There was a problem hiding this comment.
Thanks @kwcantrell! This is awesome. I have a couple of requests, but these are a lot smaller -- the main thing is asking for two corner-case tests on Empress._projectObservations() that verify these are handled correctly.
| i, | ||
| j; | ||
|
|
||
| // find "non-represented" tips |
There was a problem hiding this comment.
Not important for the scope of this PR (we probably have more urgent performance issues to tackle first), but rereading this I'm thinking that it might be possible to combine these iterations ("find not represented nodes" & "propagate assignments up to internal nodes") into a single iteration over the tree (rather than two iterations, as is being done now). May be worth opening an issue for later if you agree.
| deepEqual(columns, groups); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests look really fantastic. Having this stuff tested is going to make life so much easier!!!
This may be asking a bit much, but would it be possible to add in two extra corner-case tests to verify that these are correctly handled? These could both be on the same simple tree used for the other tests.
- All of the tips are unique to the same group -> all internal nodes, including root (
7), should be assigned to that group - All of the tips are not present in any group (i.e.
obsis empty) -> no tips or internal nodes should be assigned to a group
IMO the first test would be especially useful to have written down so we don't have to worry about it in this context, since off-by-1 errors missing the root have come up here in the past :)
removed 'addNonUnique' from _projectObservations' function description and fixed typos/format Co-authored-by: Marcus Fedarko <[email protected]>
| @@ -66,15 +66,15 @@ require(['jquery', 'BPTree', 'Empress', "util"], function($, BPTree, Empress, ut | |||
| for (var i = 0; i < groups.length; i++) { | |||
| var group = groups[i]; | |||
| var expectedArray = Array.from(expectedResult[group]); | |||
| var resultArray = Array.from(result[group]); | |||
| var resultArray = util.naturalSort(Array.from(result[group])); | |||
There was a problem hiding this comment.
I was thinking more along the lines of calling util.naturalSort() on both the expected array and result array... but I tested this and it still works, somehow ???, so whatever -- not a huge deal. JavaScript confuses me sometimes :P
This fixes #152. One thing to note is that the signature for _projectObservations has now changes. It takes one more parameter that, if set true, will insert a "non-unique" group into the returning object.