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

"Incomplete" interpolation for sequential / diverging color maps #760

Closed
fedarko opened this issue May 9, 2020 · 1 comment · Fixed by #763
Closed

"Incomplete" interpolation for sequential / diverging color maps #760

fedarko opened this issue May 9, 2020 · 1 comment · Fixed by #763
Labels

Comments

@fedarko
Copy link
Contributor

fedarko commented May 9, 2020

When selecting a sequential / diverging color map (e.g. Viridis), the interpolation is incomplete: the "end" color isn't the end of the color map. See the example below, where the end color should be #fee825 (yellow) but is actually #8cb373 (blue-ish green).

image

This notably doesn't seem to be a problem when using the "continuous values" scaling:

image

I suspect that this line in color-view-controller.js may be causing these problems, but I'm not sure.

@fedarko
Copy link
Contributor Author

fedarko commented May 9, 2020

Actually, coming back to this, the main part of the problem looks to be with the for loop in ColorViewController.getInterpolatedColors():

ColorViewController.getInterpolatedColors = function(values, map) {
map = map || 'viridis';
map = chroma.brewer[map];
var total = values.length;
var interpolator = chroma.bezier([map[0], map[3], map[4], map[5], map[8]]);
var colors = {};
for (var i = 0; i < values.length; i++) {
colors[values[i]] = interpolator(i / total).hex();
}
return colors;
};

There's an off-by-one error. If there are 5 unique values, then total will be 5, but the final color will be set on the i=4 iteration of the loop (since i is 0-indexed). I confirmed using chroma.js' fancy docs that this is causing the exact shade of blue-ish green we're seeing above:

image

This does relate to the question of why interpolator is being set the way it is (using chroma.bezier() and 5 points from within the color map, rather than just the full color map), as mentioned. If we just set the interpolator using chroma.scale(), the 4/5-th color is still closer to the expected yellow value:

image

I think a fix to this problem should address both of these issues.

fedarko added a commit to fedarko/emperor that referenced this issue May 11, 2020
Closes biocore#760. Tests are broken, though, so will need to fix those
(along with tests broken from fixing biocore#762).
fedarko added a commit to fedarko/emperor that referenced this issue May 11, 2020
fedarko added a commit to fedarko/emperor that referenced this issue May 11, 2020
Also added an empress code attribution to getInterpolatedColors()
ElDeveloper pushed a commit that referenced this issue May 16, 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.
fedarko added a commit to fedarko/emperor that referenced this issue May 29, 2020
ElDeveloper pushed a commit that referenced this issue 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
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants