biocore / emperor

Emperor a tool for the analysis and visualization of large microbial ecology datasets
http://biocore.github.io/emperor/
Other
52 stars 50 forks source link

Gradient SVG is slightly inaccurate #788

Closed fedarko closed 3 years ago

fedarko commented 3 years ago

The actual coloring of samples is fine, I think, but the SVG gradients produced sometimes cover only ~99% of the domain, not 100%. This means that the end colors shown in the gradients are very slightly incorrect.

It's pretty confusing, because right now there are two tests that test the gradient SVG creation:

  1. In one of these tests this error is present. The returned Viridis gradient, which is used as the reference for verifying the result, ends at 99% (with #f8e725), rather than at 100% (#fee825, which is the actual end color for Viridis).

  2. In the other one of these tests, this error does not seem to be present: the color map being tested is Blues, and the minimum and maximum colors (#f7fbff and #08306b) match up with the extrema for this color map (going off what chroma.scale('blues')(0) and chroma.scale('blues')(1) return).

I think the problematic part of code is within this block:

https://github.com/biocore/emperor/blob/b625a5403ce9ad0ef3a84c6bc0b68b33e97888a4/emperor/support_files/js/color-view-controller.js#L426-L439

The first for loop seems suspicious to me. I'm not completely sure yet what's wrong with this code, but it looks like there might be precision issues with the s <= max check: since the data used for the test that failed had a step of (4 - 0) / 100 = 0.04, while the data used for the test that succeeded had a relatively larger step of (50 - 0) / 100 = 0.5.

I think it would make the most sense to just remove some of this code and use chroma.scale.colors() to do the work for us:

 var stopColors = interpolator.colors(101);
// Now we can construct gradientSVG just like we did before, with the final color (corresponding
// to the maximum value) being placed at 100%

Can put together a PR for this tomorrow if you'd like.

This impacts Empress as well, since the gradient SVG code there was adapted from this code.

ElDeveloper commented 3 years ago

Thanks for tracking this down. Yes, it would be very helpful if you can put together a PR.

On (Oct-13-20|20:03), Marcus Fedarko wrote:

The actual coloring of samples is fine, I think, but the SVG gradients produced sometimes cover only ~99% of the domain, not 100%. This means that the end colors shown in the gradients are very slightly incorrect.

It's pretty confusing, because right now there are two tests that test the gradient SVG creation:

  1. In one of these tests this error is present. The returned Viridis gradient, which is used as the reference for verifying the result, ends at 99% (with #f8e725), rather than at 100% (#fee825, which is the actual end color for Viridis).

  2. In the other one of these tests, this error does not seem to be present: the color map being tested is Blues, and the minimum and maximum colors (#f7fbff and #08306b) match up with the extrema for this color map (going off what chroma.scale('blues')(0) and chroma.scale('blues')(1) return).

I think the problematic part of code is within this block:

https://urldefense.com/v3/__https://github.com/biocore/emperor/blob/b625a5403ce9ad0ef3a84c6bc0b68b33e97888a4/emperor/support_files/js/color-view-controller.js*L426-L439__;Iw!!Mih3wA!VeKZkpvzEILHoclhWPbUFi5mPuFp3ezfo9QqGvYAozXbwgUZKc2TQB04MQeUhrk$

The first for loop seems suspicious to me. I'm not completely sure yet what's wrong with this code, but it looks like there might be precision issues with the s <= max check: since the data used for the test that failed had a step of (4 - 0) / 100 = 0.04, while the data used for the test that succeeded had a relatively step of (50 - 0) / 100 = 0.5.

I think it would make the most sense to just remove some of this code and use chroma.scale.colors() to do the work for us:

var stopColors = interpolator.colors(101);
// Now we can construct gradientSVG just like we did before, with the final color (corresponding
// to the maximum value) being placed at 100%

Can put together a PR for this tomorrow if you'd like.

This impacts Empress as well, since the gradient SVG code there was adapted from this code.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://urldefense.com/v3/__https://github.com/biocore/emperor/issues/788__;!!Mih3wA!VeKZkpvzEILHoclhWPbUFi5mPuFp3ezfo9QqGvYAozXbwgUZKc2TQB04BH9AxT4$