Scale barplot thicknesses relative to tree size; makes default layout barplot lengths look thicker; fix a small Colorer bug #442
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Relevant to biocore#276. The reliance on max displacement (which doesn't seem to have an analogue in the unrooted layout? or maybe we can sort of treat it the same way as circ layout, idk) means that this might be too specific a solution to apply to line width thicknesses, but it should make using barplots a lot less painful
|
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv, just-fm.qzv, plain.qzv |
ElDeveloper
approved these changes
Nov 10, 2020
should address @ElDeveloper comment
|
Thanks @ElDeveloper! Should be addressed now. I also noticed a small bug where barplot border lengths weren't being scaled in the same way that the other barplot lengths were being scaled -- that should be fixed in 56552f5. (I also changed the default barplot length from 50 to 10, since I think borders being 5% of the tree thickness looked a bit weird -- in practice this looks similar to the previous default setting before this PR.) |
|
Thanks @fedarko! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of the work on #276. This should make barplots look more consistent between the rectangular and circular layouts, and in particular make circular layout barplots' defaults look a lot more legible. Barplot lengths, rather than being in arbitrary units, are now scaled by the tree "thickness" (root to farthest tip width in the rectangular layout, 2 * that distance for the circular layout).
Old:
New:
Note 1: currently I have this set so that the default barplot layer length, 100, is equal to 1/10th of the tree "thickness." If folks would prefer a thinner default setting, we can adjust the scaling code as needed.
Note 2: I also noticed and fixed a small problem with the Colorer module, where the "freebie" problems with barplot borders we discussed in #400 re-emerged. Basically, this was because
rgbToFloat()was being called on a GL array (with colors in the range [0, 1]), not on an RGB array (with colors in the range [0, 255]) -- so the RGB float colors didn't match. I added and tested some code to do the RGB -> GL conversion. I also bit the bullet and added a test that verifies that the "freebie" checking works (i.e. unnecessary triangles aren't being drawn), and added some extra barplot testing code.