Fix a bug with barplot length scaling; abstract and test length-scaling code #309
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
TLDR, "0.0" was getting converted to "0", and those are different things from JS' perspective in a mapping (since mapping keys are all treated as strings). By storing the initial string metadata values as the keys, we should be good. (This is already what the coloring code was doing.)
(biocore#300 is now documented on GitHub, so we can get to it eventually)
|
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv |
|
Thanks @fedarko, this looks good to me. I think it should be good to merge. |
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.
The bug in question occurs when any of the values in the feature metadata field used for length scaling are integer values represented as floats (e.g.
"0.0"). The mapping between unique numeric feature metadata values and their corresponding lengths is contained in a mapping where, previously, the keys were passed throughparseFloat()first. The problem with this is thatparseFloat("0.0")is0, and keys in Objects are treated as Strings -- so the entry for0was inaccessible to the nodes with a feature metadata value of0.0, bizarrely. (The code assumes that a value being missing as a key from that mapping means that the value in question was non-numeric... but in this case it was because the Object was created incorrectly.)Anyway, now the code stores the original string values as the keys in the Object, not the
parseFloat()-ed values. (This is the same way the color-scaling code works -- the reason that that code doesn't suffer from this bug is likely because I was able to adapt it directly from Emperor, while the length-scaling code required some extra tweaking :) The problematic code in question has also been abstracted to a new utility function, which is now decently tested.(This bug came up when testing visualizing feature importance scores as barplots -- the majority of importance scores in the
moving-pictures-classifier/feature_importance.qzafile in this tutorial are 0s, so I noticed that a lot of these were just missing from the barplot.)