Page MenuHomePhabricator

[XRAY] A Color Choosing helper for XRay Graph
ClosedPublic

Authored by varno on Jan 31 2017, 6:25 PM.

Details

Summary

In Preparation for graph comparison, this patch breaks out the color
choice code from xray-graph into a library and adds polynomials for
the Sequential and Difference sets from ColorBrewer.

Depends on D29005

Diff Detail

Repository
rL LLVM

Event Timeline

varno created this revision.Jan 31 2017, 6:25 PM
varno updated this revision to Diff 86562.Jan 31 2017, 7:31 PM
  • respond to comments
varno updated this revision to Diff 86567.Jan 31 2017, 8:30 PM
  • add in generation script
dblaikie added inline comments.Feb 1 2017, 10:02 AM
tools/llvm-xray/xray-color-helper.cc
524–526 ↗(On Diff #86567)

usually skip the {} on single line blocks

tools/llvm-xray/xray-color-helper.h
23 ↗(On Diff #86567)

I think this is probably google style clang-tidy (one space indenting the private/protected/etc) - don't think it's LLVM's style?

dberris requested changes to this revision.Feb 2 2017, 10:04 PM
dberris added inline comments.
tools/llvm-xray/xray-color-helper-gen.wls
1–4 ↗(On Diff #86567)

Probably needs top-matter to bear the correct license.

tools/llvm-xray/xray-color-helper.cc
55 ↗(On Diff #86567)

Formatting suggestion:

static constexpr double Coeffs[][3][21] = {
  { // Describe 2D array
    {  // Describe this array
      -6.878460665e6, ...
       ...
    },
    {  // Describe this array
      -2.54152823e6, ...
      ...
    },
    ...
  },
  ...
};
tools/llvm-xray/xray-color-helper.h
22 ↗(On Diff #86567)

This would benefit hugely from documentation. Show how to use it, what to use it for, etc.?

60 ↗(On Diff #86567)

I might be missing something, but would it work better if you returned an RGB tuple?

This revision now requires changes to proceed.Feb 2 2017, 10:04 PM
varno updated this revision to Diff 87341.Feb 6 2017, 5:28 PM
varno edited edge metadata.
  • Address Reviewer comments
varno marked 2 inline comments as done.Feb 6 2017, 5:30 PM
varno added inline comments.
tools/llvm-xray/xray-color-helper.h
60 ↗(On Diff #86567)

Not really, I would just have to convert it later, and the format specification is a little hard. If someone wants raw RGB tuples (unlikely) then they can add another function to do so easily enough. But as it is I would never use such a function.

varno updated this revision to Diff 87348.Feb 6 2017, 6:02 PM
  • Address Reviewer comments
  • clang-format
dberris requested changes to this revision.Feb 7 2017, 1:08 AM
dberris added inline comments.
tools/llvm-xray/xray-color-helper.cc
39 ↗(On Diff #87348)

Is this first polynomial for the "red" component? If so, document that clearly here (both to clearly mark the start of the array, and as an indentation guide).

398 ↗(On Diff #87348)

Explain what "Diverging Polynomials" means. Really this whole file shouldn't assume that readers will have time to look at the mentioned website just to understand what the names/mnemonics/short-cuts mean.

581–589 ↗(On Diff #87348)

Reading this, it looks like you do just want to give a tuple<uint8_t, uint8_t, uint8_t> as the result, then the caller can transform it where they need it. Right now it's assuming that the caller will always want the string value rather than the triple, which seems like a bad assumption to make.

This revision now requires changes to proceed.Feb 7 2017, 1:08 AM
varno updated this revision to Diff 87738.Feb 8 2017, 5:08 PM
varno edited edge metadata.
  • Address Reviewer comments
  • clang-format
  • addres reviwer comments
varno updated this revision to Diff 87739.Feb 8 2017, 5:15 PM
  • fix comments
dberris requested changes to this revision.Feb 9 2017, 12:01 AM

Almost there...

tools/llvm-xray/xray-color-helper.cc
632–638 ↗(On Diff #87739)

Does the initialisation have to happen in the constructor body, or could some (or all) of these go on an initialiser list instead?

665 ↗(On Diff #87739)

Should also probably be a const member function.

677 ↗(On Diff #87739)

Should probably be a const member function.

39 ↗(On Diff #87348)

I hate to do this again, but this would look better if the comment was the same line as the { and the numbers were indented after the brace. Like so:

{ // Red coefficients
  -6.878...,
},
{ // Green coefficients
  -2.545...,
},
This revision now requires changes to proceed.Feb 9 2017, 12:01 AM
varno updated this revision to Diff 87942.Feb 9 2017, 7:04 PM
varno edited edge metadata.
  • Address Reviewer comments
  • clang-format
  • addres reviwer comments
  • fix comments
  • Address Reviewer comments
varno updated this revision to Diff 87955.Feb 9 2017, 7:57 PM

Git Rebase

dberris accepted this revision.Feb 9 2017, 9:47 PM
This revision is now accepted and ready to land.Feb 9 2017, 9:47 PM
chandlerc requested changes to this revision.Feb 15 2017, 6:22 PM
chandlerc added a subscriber: chandlerc.

Sorry to jump in with a peanut gallery comment, but just wanted to push back on some of the complexity we're adding here.

tools/llvm-xray/xray-color-helper.cc
22–35 ↗(On Diff #87955)

I really think this is more complexity than is reasonable to add to LLVM for this use case.

IMO, we don't need perfectly accurate color interpolation here. We need roughly accurate, *simple* interpolation so that it is useful even where it isn't perfect.

I would strongly suggest switching to what I understand is the very common scheme of RGB -> HSV, linear interpolate, HSV -> RGB process for interpolating colors. That seems likely to be good enough for all realistic usages LLVM will have, and much easier to understand and maintain going forward.

This revision now requires changes to proceed.Feb 15 2017, 6:22 PM
varno added inline comments.Feb 20 2017, 1:51 AM
tools/llvm-xray/xray-color-helper.cc
22–35 ↗(On Diff #87955)

Hi Chandler.

Colors are hard. Really hard. Especially when you want perceptual uniformity, as we do here. We want our perception of color to present a sense of distance quality. This is an attempt to have good perceptual uniformity in the least complicated way possible.

I have not heard of anyone interpolating in HSV, can you show an example. Everyone who does this sort of thing interpolates either in RGB very, very carefully (with many had chosen intermediate pointe) or in LAB like I am doing (or using bisection in delta-E).

varno requested review of this revision.Feb 21 2017, 5:47 AM
varno edited edge metadata.
varno planned changes to this revision.Feb 21 2017, 4:04 PM
varno updated this revision to Diff 89435.Feb 22 2017, 3:47 PM
  • [XRAY] A Color Choosing helper for XRay Graph
  • Address Reviewer Comments and moved to piecewise HSV interpolation
varno marked an inline comment as done.Feb 22 2017, 3:48 PM
varno updated this revision to Diff 89436.Feb 22 2017, 3:55 PM
  • clang-format
chandlerc added inline comments.Feb 22 2017, 3:58 PM
tools/llvm-xray/xray-color-helper.cc
22–35 ↗(On Diff #87955)

I understand that colors are hard. I just don't believe that it is actually important to have (perfect) perceptual uniformity for these particular use cases.

I think that for tools like this, being close or having a rough correspondence is more than enough. If someone wants to get precise, they should use the actual data and not colors. The colors are to help *at a glance*.

Interpolating using HSV is the first answer to the first SO question I find when I search on Google for "interpolating between two colors":
http://stackoverflow.com/questions/13488957/interpolate-from-one-color-to-another

So, I guess not everyone?

Still, my comment remains in essence: this is too complicated for the value it brings. We should use a simpler solution with lower quality because the qualitative difference won't matter here. SO suggests one approach that seems reasonably simple. I'm happy for you to suggest other approaches, but they need to be similarly simple.

varno added inline comments.Feb 22 2017, 4:28 PM
tools/llvm-xray/xray-color-helper.cc
22–35 ↗(On Diff #87955)

Hi Chandler

This patch implements the interpolation in HSV as requested.

chandlerc added inline comments.Feb 22 2017, 4:58 PM
tools/llvm-xray/xray-color-helper.cc
22–35 ↗(On Diff #87955)

Thanks, this is already tremendously simpler.

I'd also suggest starting with a fairly small set of colors / schemes. I think 2 or 3 are fine. IT is easy to add more if and when we have a compelling use case for more color schemes. Until then, YAGNI -- we shouldn't add the code.

varno updated this revision to Diff 89464.Feb 22 2017, 7:50 PM
  • Reduce number of available ColorMaps as requested

(FWIW, I like the more minimal schemes here. We have a couple of sequential ones, and a solid divergent ones. Enough to be very useful, but no more than we really have use cases for today. That part LGTM, but leaving the code review proper to others.)

varno marked 15 inline comments as done.Feb 22 2017, 8:35 PM
dberris accepted this revision.Feb 24 2017, 1:04 PM

LGTM -- thanks very much for making the changes. I'll test and land soon.

This revision was automatically updated to reflect the committed changes.