Some (mostly coloring-focused) bug fixes #763
Merged
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
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 :)
wasade
reviewed
May 15, 2020
wasade
reviewed
May 15, 2020
ElDeveloper
approved these changes
May 15, 2020
There was a problem hiding this comment.
Looks good and ready to be merged once @wasade's comments are addressed.
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.
|
Comments have been addressed. It's worth noting that scientific notation numbers like
... where the colors are assigned correctly, but the legend is out of order. If y'all would like me to I can try to address #761 in this PR as well, but I'm not sure where to look for that. Thanks! |
ElDeveloper
pushed a commit
that referenced
this pull request
May 29, 2020
* BUG: Make naturalSort treat +/-Infinity as words Modified the function and added a unit test verifying this. Both modifications are from Empress' codebase, specifically in the PR biocore/empress#159. * MNT: Update Chroma.js to v2.1.0 (from v1.1.1) #762 As expected, this causes the JS tests to fail due to small precision differences. Will need to update those. * BUG: Fix interpolation for "ordinal" color scaling Closes #760. Tests are broken, though, so will need to fix those (along with tests broken from fixing #762). * MNT: use "Viridis" instead of "viridis" as default 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). * TST: Fix getInterpolatedColors() test for #760 * TST: More CVC test fixes for #760/#762 Also added an empress code attribution to getInterpolatedColors() * TST: Partial work on fixing expected gradient SVGs emphasis on "partial", would prefer to automate this * TST: Update gradient SVGs to use new colors 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 #762 :) * MNT: only call parseFloat() once in naturalSort() Addresses comment from @wasade * TST: Test naturalSort() with sci. notation numbers Addresses @wasade's comment. NOTE that #761 is still gonna mess things up, though. Will comment accordingly in #763. * BUG: Sort field vals in DecompView.setCategory Fixes #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. * TST: Add infrastructure for #761 regression test Still need to, like, actually set up a data JSON to provide to setCategory(), but I don't think that should be too bad * TST: Actually test setCategory() order #761 PR should be doable now * MNT: Fix inconsistencies with "var obs" in DV tsts 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. * REL: Document #760, #761, #762 in changelog * STY: adapt to the fickle whims of gjslint (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) * TST: Remove "scripts/*.py" from flake8 travis call ...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.
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.
Closes #760 and closes #762. Also fixes that
naturalSort()Infinity issue we talked about (there isn't an open issue for that though).The JavaScript tests have
mostlyall been updated. This involved updating both the expected colors from the interpolation fixes, as well as updating slightly different colors due to the new Chroma.js version. (For the two tests that directly checked gradient SVG equality, I elected to just copy in the new expected SVG over the old expected SVG markup -- changing each hex color manually seemed kinda daunting, as shown in the QUnit diff below.)I also tested this manually, and can confirm that this resolves #760 -- see below, where the interpolated Viridis colors now extend to cover the full color map.