VEuPathDB / web-monorepo

A monorepo that contains all frontend code for VEuPathDB websites
Apache License 2.0
2 stars 0 forks source link

Continuous overlay range #16

Closed moontrip closed 1 year ago

moontrip commented 1 year ago

Making a new PR at web-monorepo. Detailed discussions were made at https://github.com/VEuPathDB/web-eda/pull/1696.

moontrip commented 1 year ago

Following your request, I made a PR at web-monorepo, @bobular

moontrip commented 1 year ago

Hi @bobular I have updated this PR based on our discussions and your suggestions at https://github.com/VEuPathDB/web-components/issues/454.

moontrip commented 1 year ago

@bobular Thank you for your comments! 👍 I think that you are right. Perhaps that part, ? defaults.displayRangeMin != null && defaults.displayRangeMax != null may not be necessary. I will try to check it out carefully. Will also add a comment about lodash.min/.max

moontrip commented 1 year ago

@bobular I have tried your suggestions and didn't notice specific issue. So, a commit is made to remove that part & add a comment concerning lodash.min/.max

moontrip commented 1 year ago

Thank you for your detailed reviews and tests @bobular 👍 - Didn't have time to reply earlier due to map stuff we worked together 😄

I will change the type (typo haha). Also will try to adjust the legend. By the way, here is one question concerning the legend you suggested. Do we need to use legend for the calculated range or for the absolute max based range? I think that your intention is the former but want to make sure of it.

bobular commented 1 year ago

Hi @moontrip - I'm not sure I understand the question but here is my understanding of how the legend needs to be drawn:

The min and max values displayed on the legend (e.g. -30 and +20 in my screenshot above) are correctly sourced, I think.

The transform applied to generate the colours in the legend needs to be the same as used in the viz component, e.g. -maxAbs to +maxAbs. Maybe pass the normalize object to the legend if possible? This would avoid having to code the logic twice.

bobular commented 1 year ago

I looked at the PlotGradientLegend implementation. Currently the whole colour scale is rendered as an SVG linearGradient. I think we need to pass in to PlotGradientLegend the xxxxColorScaleMap and the normalizer, and use those to generate the gradient stop point colours from "raw" input values legendMin to legendMax

moontrip commented 1 year ago

@bobular Thank you for sharing your insights 👍 I will think of it next week. Have a great weekend!

moontrip commented 1 year ago

Hi @bobular I have implemented new colormap based on range for divergent, sequential, and sequential reversed. For your better understanding when reviewing, I summarized how to resolve this in the following.

a) at ScatterplotVisualization, RGB values for overlayMin and overlayMax via gradientDivergingColorscaleMap(normalize(overlayMin/Max)) are passed to PlotLegend/plotGradientLegend. b) at PlotGradientLegend, computing a kind of bins, which depends gradient type (e.g., divergent, sequential, etc.), range used for normalize function, and the number of gradient colors, and then find an array index with a criterion - this also depends on gradient type (either overlayMin or overlayMax). It is because sorting RGB colors is very difficult. c) the array index is utilized to determine 1) trimming down pre-existing gradient colors and 2) where to place overlayMin's RGB or overlayMax's RGB in the trimmed gradient colors.

Here are screenshots to show current Live site and new divergent colormap (GEMS1 data): I think sequential-related colormaps would also work fine with the logic I implemented but couldn't find appropriate example to demo (mostly starting from 0 or 1). Note that displayRangeMin/Max = (-30, 20) and rangeMin/Max=(-15.3, 273.91) from the variable's metadata, thus final range becomes (-30, 273.91).

a) current Live site: incorrect range as you initiated in the ticket

divergent colormap01-LiveSite

b) new divergent colormap: as you can see, the color of the min does not start from dark blue

divergent colormap01

moontrip commented 1 year ago

@bobular After checking the cases of the sequential and sequential reversed, I don't think we need to use my logic for them. So, I simplified the logic and changed accordingly. Now we only use the logic for the divergent case.

bobular commented 1 year ago

Converting to draft as I'm not sure of the status of this branch. Currently reviewing https://github.com/VEuPathDB/web-monorepo/pull/44

moontrip commented 1 year ago

@bobular yes you are right I will close this PR 👍 https://github.com/VEuPathDB/web-monorepo/pull/44 will take over instead of this.