Support circular layout barplots; show legends for barplots using length-scaling #357
Conversation
how is this actually not that bad what
since this kinda uh crashes the EMP tree LMAO
a more elegant way of representing this for circ barplots
I would still like to add some tests to the coordinate functions, but I think this is sufficient to close biocore#297.
just gotta test y-coords, with a similar switch stmt
also add approxEqual func which I FEEL LIKE I ALREADY ADDED but w/e
There was a problem hiding this comment.
This looks great thanks @fedarko. Just a couple of notes!
I just noticed something a bit off with the bar scaling, maybe worth fixing here? When you click on the scale checkbox and you have a length different to 100 (the default), the maximum length is set by default to 100. I think this should by default be the maximum length so that bars don't shrink out of the blue. See this animated gif:
Thoughts?
That's a really good point, and I agree that the UI for this is a bit confusing. Part of the challenge here is that, if the user has already done some length-scaling stuff for a layer, I'm not sure we want to mess with that when the user switches back to non-length-scaling -- do you think an acceptable compromise would be setting the "max length" for scaling to the default length the first time the "length scaling" checkbox is checked? Or maybe we should just update it every time. I guess in the same vein we should definitely synchronize the default lengths between feature and sample metadata barplots --
I'll open up a new issue for this if that sounds ok with you; I think fixing this would be mildly out of scope for this PR. |
Thinking more about this. I think this is only a big problem for this tree that has an unusually long branch. Let's table this until after we sort out consistent lengths, etc |
There was a problem hiding this comment.
The circular bar plots look really nice @fedarko! I just have a few comments.
Forgot to update the docs. Thanks @kwcantrell for pointing this out. Also changed a small block of code to just compute thisLayerMaxD once and not like 3 times (?? why was I doing that lol)
So that this is only done once per layout. Addresses comment from @kwcantrell.
... and not stringification :P Default epsilon is 1e-5, but for a few tests I only bothered writing things out to 4 decimal places, so for these I use 1e-4.
|
OK, all comments should be addressed now. Thanks @kwcantrell and @ElDeveloper! |
|
Just a quick update -- I noticed that there's a bug in the master version of Empress where searching for an invalid node name wouldn't change the styling of the search bar and would instead throw an error:
It turns out this is because |
There was a problem hiding this comment.
Things look good @fedarko! I just added one minor comment but otherwise should be good to go. @kwcantrell feel free to merge whenever you get a chance to review.
|
Thanks @ElDeveloper and @kwcantrell! Comments have been addressed, and I merged in the #360 changes now in master. edit: also just added in a |
|
Thanks @fedarko! |
|
Thanks both! |
This PR replaces #346 (this is the branch I ended up resolving the conflicts in). It closes #354 (simple "legends" are now shown for all barplot layers using length scaling, showing the min/max values in a field), and since this includes all of #346's stuff this also closes #297 (circular layout barplots) and #343 (make the user press
Updateto draw barplots, rather than having theDraw barplotscheckbox immediately show barplots).Copying the text of #346 and its comments below. Many of the points brought up are still relevant (barplot lengths are still arbitrary, etc.).
Closes #297 and closes #343.
This should probably not be merged until #345 and #340 are, but this branch does work in case people want to use this functionality.
Thanks!
Issues we should probably discuss
Barplot lengths are still in arbitrary units, so a barplot with length "100" looks thinner in the moving pictures circular layout than in the rectangular layout. Standardizing these somewhat should be possible, but I wanted to avoid doing that until Layout js #345 is in (to avoid having to redo the work to accommodate JS layouts).
Circular layout bars are drawn here by drawing two rectangles (reusing an image I posted here) --
We may want to smooth this out, and look at the number of tips in the tree to determine how many rectangles to draw (e.g. use ~15 rectangles for tiny trees like the one above, and only 1 rectangle for massive EMP-scale trees). Or we could just leave this as is until the future, when we'd ideally have a shader draw these as actual curved sections.
Although the functions that add the coordinates for individual bars (in both rect/circular layouts) are now tested,
addFMBarplotLayerCoords()andaddSMBarplotLayerCoords()are still untested. Testing these is going to be a bit time-consuming -- testing the trig stuff gets really annoying really quickly (see e.g. the_addCircularBarCoords()test here...), so I think it makes sense to defer testing these until we agree on how many rectangles to use for approximating a curve (per the above point).Comments from #346
Is it possible to use a fraction of the spatial length of an "average branch" as a unit? This could then be computed per layout and things should be roughly equivalent at least relative to the tree.
You could for example find the mean branch length, and then find whatever branch is closest to that value, look at the euclidean distance of the vertices (from the layout data) for that branch and take a fraction of that value. Maybe I'm missing something but it seems like what we need is a frame of reference.
Originally posted by @ElDeveloper in #346 (comment)
I think this could work. This boils down to the same problem as with line width (#276) -- I think trying to see how "long" a given node of a known length is in one layout, and then using that length somehow when scaling barplot lengths / line widths, could work.
Actually -- thinking about this some more ... So our goal here is making sure that the drawn length (in terms of "outer radius" - "inner radius" of a barplot layer with length L is proportional to the drawn length (in terms of "rightmost x - innermost x") of another barplot layer in the rectangular layout with the same length. These measures both only involve a single "axis" (x in the rectangular layout, radius in the polar coordinates for the circular layout). What we could do is use the drawn length of the maximum-displacement node (the node with the rightmost/max x coordinate for the rectangular layout, the node with the max radius for the circular layout) as a "unit" for drawing bars -- so maybe a user-specified length of "100" is equal to 1/10 of that length, or something similar. ...Maybe?
I'll think about this more next week. I have a suspicion that this scaling stuff isn't really that bad, but every time I try to think about it my brain goes to mush :P
Originally posted by @fedarko in #346 (comment)