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

Draw barplots in the circular layout #346

Closed
wants to merge 36 commits into from

Conversation

fedarko
Copy link
Collaborator

@fedarko fedarko commented Aug 21, 2020

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) --
    image
    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() and addSMBarplotLayerCoords() 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).

fedarko added 30 commits August 19, 2020 19:42
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
@emperor-helper
Copy link

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv

@ElDeveloper
Copy link
Member

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 #345 is in (to avoid having to redo the work to accommodate JS layouts).

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.

@fedarko
Copy link
Collaborator Author

fedarko commented Aug 22, 2020

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 #345 is in (to avoid having to redo the work to accommodate JS layouts).

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.

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

@fedarko
Copy link
Collaborator Author

fedarko commented Aug 28, 2020

Closing in favor of #357.

@fedarko fedarko closed this Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants