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

Interpolate test colors between red and green #1827

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snicolet
Copy link
Member

@snicolet snicolet commented Oct 6, 2023

closes #1827

@Sopel97
Copy link
Member

Sopel97 commented Oct 6, 2023

Interpolating RGB colors is a bad idea in itself, but this doesn't even account for gamma. See https://www.alanzucconi.com/2016/01/06/colour-interpolation/

@snicolet
Copy link
Member Author

snicolet commented Oct 6, 2023

Interpolating RGB colors is a bad idea in itself, but this doesn't even account for gamma. See https://www.alanzucconi.com/2016/01/06/colour-interpolation/

This comment is over-aggressive, if you ask me.

  1. I knew about gamma correction even before you were born
  2. what I'm trying here is to bring a little bit of life in the long list of active tests on Fishtest, not writing a color library for Python.

It is really a joy to propose improvements in this community, where the most common feedback is contempt...

@Disservin
Copy link
Member

Sopel's comment could have been better formulated but then answering with

I knew about gamma correction even before you were born

is not a nice response either.

It would be nice if you included some images in your pr to highlight the visual changes, though.

@Sopel97
Copy link
Member

Sopel97 commented Oct 6, 2023

Sorry, didn't mean it this way, maybe it came like that because I tried to be concise. I wanted to say that the current state it will be pretty eye jarring when interpolating colors that are very far, like green with red, and the interpolated colors will have little to no value for the viewer. It's also unclear how it will play with the fixed font color.

@peregrineshahin
Copy link
Contributor

It would be nice if you included some images in your pr to highlight the visual changes, though.

There is always the posibility of testing UX patches on DEV enviroment, Even on production much worse ideas and suggestions was either updated on production to gather feedback without merging or with merging and reverting, we welcome experimenting changes via PR's.

@peregrineshahin
Copy link
Contributor

peregrineshahin commented Oct 6, 2023

You know, bulding fishtest from scratch and connecting workers to it is a time/energy-consuming task.

@Disservin
Copy link
Member

Disservin commented Oct 6, 2023

There is always the posibility of testing UX patches on DEV enviroment

sure, but if a pr includes an "example" it's a) much easier for everyone to view and judge initially (ofc having a real example page is much better and realistic) b) who knows when dev will be tested with this...

@peregrineshahin
Copy link
Contributor

peregrineshahin commented Oct 6, 2023

sure, but if a pr includes an "example" it's a) much easier for everyone to view b) who knows when dev will be tested with this...

As I said, to test in place one needs to install ubuntu and update system and build fishtest from scratch and connect workers and then see, I don't expect devs to be responsible of doing that without an easy and accessable enviroment to all rather than locally.

For "b) who knows when dev will be tested with this..."
If this is not tested on DEV then the maintainer has other PR's in mind first and thus the concern of such "bad ideas" to sneak in master has no bases :D

@vdbergh
Copy link
Contributor

vdbergh commented Oct 6, 2023

Let me say that personally I dislike this, although I see no hard objections against it.

@vdbergh
Copy link
Contributor

vdbergh commented Oct 6, 2023

RGB interpolation is not the best thing to do. E.g. https://www.alanzucconi.com/2016/01/06/colour-interpolation/

@peregrineshahin
Copy link
Contributor

I think you linked the same link sopel linked, and I think that we should not jump to conclusions before actually seeing it

RGB interpolation is not the best thing to do. E.g. https://www.alanzucconi.com/2016/01/06/colour-interpolation/

@ppigazzini
Copy link
Collaborator

ppigazzini commented Oct 6, 2023

DEV updated, joined some workers. It seems to work in light mode.

image

@ppigazzini ppigazzini added enhancement server server side changes gui labels Oct 6, 2023
@ppigazzini
Copy link
Collaborator

ppigazzini commented Oct 6, 2023

The solution here is to interpolate the color of the 0 LLR, that is over the gray line in a 3d color space for either the light or dark theme, with the saturated Green or Red of the finished test. To be honest a simple RGB interpolation should work mostly fine if done with the gray line.
So that link is not really useful for the intended task of the PR, that is very simple. The perceived color sensation of an ROI of the screen is always biased from the colors of the surrounding ROIs, so working in the CIE Lab perceptively uniform space makes sense only in limited cases: when making the color transformation in different color space device depended (different media: screen, paper etc), or when measuring (Delta E CIE Lab) the color variation of a manufact during the time. Finally, I doubt that the typical fishtest user is using a calibrated and profiled professional monitor with a HW LUT (Eizo Coloredge at example), to appreciate that the colors are uniformly spaced.

@ppigazzini
Copy link
Collaborator

ppigazzini commented Oct 6, 2023

In effect the simple RGB interpolation works not so bad.

image

@ppigazzini
Copy link
Collaborator

ppigazzini commented Oct 6, 2023

def color_run(run, WLD):
    color = ""
    sprt = run["args"].get("sprt")

    if sprt:
        llr = float(sprt["llr"])
        low = float(sprt["lower_bound"])
        up = float(sprt["upper_bound"])
        percent = 2 * max(0, min(1, (llr - low) / (up - low))) - 1

        # rejection color
        # yellow (#FFFF00) or red (#FF6A6A)
        color_low = "#FFFF00" if WLD[0] > WLD[1] else "#FF6A6A"

        # acceptation color
        # blue (#66CCFF) or green (#44EB44)
        color_up = (
            "#66CCFF"
            if (float(sprt["elo0"]) + float(sprt["elo1"])) < 0
            else "#44EB44"
        )

        color = color_low if percent < 0 else color_up
        # calculate interpolated color
        color = interpolate_color("#F5F5F5", color, abs(percent))

    return color

@vdbergh
Copy link
Contributor

vdbergh commented Oct 7, 2023

I am puzzled by the appearance of yellow though
Screenshot from 2023-10-07 09-53-18

@ppigazzini
Copy link
Collaborator

Are you puzzled because that test should not be yellow or because it is not the best scale of yellow? If it is for the latter reason: RGB interpolation is simple and fast but does not guarantee hue. One of my points was that the right way to interpolate is between the gray level of 0 LLR and the color of finished test, changing "saturation" in cylindrical coordinates or "chroma" in cone bicone coordinates. Another it was that CIELab is theoretical correct but useful only if the user is viewing with a monitor calibrated (luminance, white point, gamma) and profiled (icc), with an OS/application using a CMS, and this only if viewing a simple image with few color patches with the same hue.

However, this PR works only with light theme, and this could be the real stopper.

@vdbergh
Copy link
Contributor

vdbergh commented Oct 7, 2023

I am surprised that the LLR of the yellow test is between the LLRs of reddish tests. This is counter intuitive.

@ppigazzini
Copy link
Collaborator

ppigazzini commented Oct 7, 2023

I am surprised that the LLR of the yellow test is between the LLRs of reddish tests. This is counter intuitive.

Tertium datur :)

If that Red/Yellow condition is correct, I suppose that the LLR should be continuous when WLD[0] ~ WLD[1] and the run could switch back and forth to Red/Yellow according to the new uploaded games results. This should be even more puzzling for the user.

        # rejection color
        # yellow (#FFFF00) or red (#FF6A6A)
        color_low = "#FFFF00" if WLD[0] > WLD[1] else "#FF6A6A"

@vdbergh
Copy link
Contributor

vdbergh commented Oct 7, 2023

Ok I see. Personally I think that yellow has no meaning in general. But it should definitely not be applied to a running SPRT test!

@XInTheDark
Copy link

Personally I dislike this patch. In the main page the tests are already sorted according to LLR. It's unnecessary IMO to add this cosmetic change.

Also, it seems misleading to e.g. show a -2 LLR test as red, or a +2 LLR as green, as it's not meaningful to make predictions about the sprt based on the current LLR alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement gui server server side changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants