equinor / webviz-subsurface-components

Custom subsurface visualizations for use in Webviz and/or Dash.
https://github.com/orgs/equinor/projects/24
Mozilla Public License 2.0
35 stars 39 forks source link

feat: ColorMapFunction type in WellLogView #2249

Open iWowik opened 1 week ago

iWowik commented 1 week ago

BREAKING CHANGE:

w1nklr commented 1 week ago

Does it make sense to specify both color function array AND color table array ? What is the behavior if both are provided (the API allows it) ? Can the design use one single property to handle both functions and table ?

iWowik commented 1 week ago

Does it make sense to specify both color function array AND color table array ? What is the behavior if both are provided (the API allows it) ? Can the design use one single property to handle both functions and table ?

@w1nklr Color table array could be easily read/written from/to a JSON file. Color function array could not be written. So it is convenient to have two different array of homogeneous elements.

If both color table and color function are referenced in a template then color table is used.

If we want to have the only reference for both color sources then we should mix color tables and color functions in a one array of inhomogeneous elements. In this case we would use the old names for references in a template (colorTable, inverseColorTable) But such names become a little misleading. Should they be renamed in something like colorTableOrFunction, inverseColorTableOrFunction?

w1nklr commented 1 week ago

Does it make sense to specify both color function array AND color table array ? What is the behavior if both are provided (the API allows it) ? Can the design use one single property to handle both functions and table ?

@w1nklr Color table array could be easily read/written from/to a JSON file. Color function array could not be written. So it is convenient to have two different array of homogeneous elements.

If both color table and color function are referenced in a template then color table is used.

If we want to have the only reference for both color sources then we should mix color tables and color functions in a one array of inhomogeneous elements. In this case we would use the old names for references in a template (colorTable, inverseColorTable) But such names become a little misleading. Should they be renamed in something like colorTableOrFunction, inverseColorTableOrFunction?

OK. I won't fight to get one single prop to handle both the tables and the function (even if I think it's not a good API). But if you want to keep 2 props; please make sure to have a super clear doc. The doc should not leave any doubt about the behavior if both props are defined.

iWowik commented 1 week ago

OK. I won't fight to get one single prop to handle both the tables and the function (even if I think it's not a good API). But if you want to keep 2 props; please make sure to have a super clear doc. The doc should not leave any doubt about the behavior if both props are defined.

@w1nklr, I like your idea to combine color tables and color functions. Because it simplify code. But the only question is how to name array and references to its elements:

w1nklr commented 4 days ago

OK. I won't fight to get one single prop to handle both the tables and the function (even if I think it's not a good API). But if you want to keep 2 props; please make sure to have a super clear doc. The doc should not leave any doubt about the behavior if both props are defined.

@w1nklr, I like your idea to combine color tables and color functions. Because it simplify code. But the only question is how to name array and references to it elements:

* Leave them as they are: colorTables for array and  colorTable, inverseColorTable for references, or

* Change them to reflect the possibility of using not only the table, but also the function

Let's use colorFunc, that can be a color function or a color table. It's not the best name, but colorFunc is already used for maps, either as a color function or a plain color value. I'll probably extend it to include the colorTable to have an unified naming for our API.

hkfb commented 3 days ago

Looks like there is a prettier formatting issue in the python wrapper code.

hkfb commented 1 day ago

@iWowik It appears that the "feat!:" prefix doesn't work after all, so I changed it back to "feat:". According to the spec, the "BREAKING CHANGE:" in the description should trigger a major release bump, so let's keep that and see if it works.

iWowik commented 1 day ago

Looks like there is a prettier formatting issue in the python wrapper code. Done

iWowik commented 1 day ago

@hkfb I have no rights to set Labels for PR. I think they would be set image