-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fix color sorting in the legend #765
Conversation
Modified the function and added a unit test verifying this. Both modifications are from Empress' codebase, specifically in the PR biocore/empress#159.
As expected, this causes the JS tests to fail due to small precision differences. Will need to update those.
Closes biocore#760. Tests are broken, though, so will need to fix those (along with tests broken from fixing biocore#762).
Both seem to be accepted by Chroma.js, but may as well be consistent with what their docs use (also, the Emperor docstrings said "Viridis" in spite of this not being followed).
Also added an empress code attribution to getInterpolatedColors()
emphasis on "partial", would prefer to automate this
Just did this by copying in the new output (finagled it to roughly match the old expected output's formatting for purposes of making the diff cleaner). Now that tests pass, I'm gonna say that this officially closes biocore#762 :)
Addresses comment from @wasade
Addresses @wasade's comment. NOTE that biocore#761 is still gonna mess things up, though. Will comment accordingly in biocore#763.
Fixes biocore#761. May be worth adding tests at some point, but since this change didn't break any tests I guess this would require writing new tests rather than updating any current ones.
Still need to, like, actually set up a data JSON to provide to setCategory(), but I don't think that should be too bad
PR should be doable now
Long story short, some tests declared "var obs;" and then didn't do anything with obs, while other tests assigned something to obs without actually declaring it first. I tried to make things consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
], | ||
percents_explained: [80.0, 20.0] | ||
}; | ||
var md_headers = ['SampleID', 'DeliberatelyAnnoyingField']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆
@ElDeveloper done, thanks! |
(for some reason it decided to get angry at using ""s instead of ''s, the version being downloaded probably got updated or something since this stuff was passing last week)
...since there is no scripts/ folder in Emperor any more. This is really bizarre, though -- this was definitely not failing last week: see https://travis-ci.com/github/biocore/emperor/jobs/339150405. It looks like the flake8 version was updated between python 3.6 builds on Travis (3.7.9 to 3.8.2), but I have v3.7.9 installed on my computer and running the flake8 command here still gives me an error about scripts/ not being a real directory. Uh, I think the path of least resistance is just fixing this for right now, but I'm very confused.
For some reason, it looks like the gjslint / flake8 versions used on Travis changed between last week and this week -- hence why the build was failing. Things should be fixed now (most of the complaints were minor things like double vs. single quotes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much @fedarko!
Closes #761; see #761 (comment) for details on the way I fixed the bug.
Along with the fix to
DecompositionView.setCategory()
, I also added in a test to make sure that these changes remain in effect. Also made some minor code fixes intest_decomposition_view.js
.Hope this is useful!