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

feat: ColorMapFunction type in WellLogView #2249

Merged
merged 42 commits into from
Oct 3, 2024

Conversation

iWowik
Copy link
Contributor

@iWowik iWowik commented Sep 17, 2024

BREAKING CHANGE:

  • colorMapFunctions array replaces colorTables array to combine color tables and functions in single array
  • colorMapFunctionName and reverseColorMapFunctionName references replace colorTable and reverseColorTable references (JSON files also should be changed)

@w1nklr
Copy link
Collaborator

w1nklr commented Sep 18, 2024

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
Copy link
Contributor Author

iWowik commented Sep 20, 2024

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
Copy link
Collaborator

w1nklr commented Sep 20, 2024

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
Copy link
Contributor Author

iWowik commented Sep 20, 2024

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:

  • 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

@w1nklr
Copy link
Collaborator

w1nklr commented Sep 24, 2024

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.

@iWowik iWowik marked this pull request as ready for review September 25, 2024 04:54
Copy link
Collaborator

@hkfb hkfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a breaking API change, suggest using the "feat!" prefix in the title and a "BREAKING CHANGE: [describe what is breaking]" in the description.

.gitignore Outdated Show resolved Hide resolved
@hkfb
Copy link
Collaborator

hkfb commented Sep 25, 2024

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

@iWowik iWowik changed the title feat: ColorFunction type in WellLogView feat!: ColorFunction type in WellLogView Sep 25, 2024
@hkfb hkfb added well-log-viewer enhancement New feature or request labels Sep 27, 2024
@iWowik iWowik changed the title feat: ColorFunction type in WellLogView feat: ColorMapFunction type in WellLogView Sep 27, 2024
example-data/facies3wells.json Outdated Show resolved Hide resolved
example-data/facies3wells.json Outdated Show resolved Hide resolved
@w1nklr w1nklr merged commit 96bcab4 into equinor:master Oct 3, 2024
10 checks passed
hkfb pushed a commit that referenced this pull request Oct 3, 2024
# [1.0.0](https://github.com/equinor/webviz-subsurface-components/compare/wsc-common@0.8.9...wsc-common@1.0.0) (2024-10-03)

### Features

* ColorMapFunction type in WellLogView ([#2249](#2249)) ([96bcab4](96bcab4)), closes [#2054](#2054) [#2259](#2259)

### BREAKING CHANGES

* - colorMapFunctions array replaces colorTables array to combine color
tables and functions in single array
- colorMapFunctionName and reverseColorMapFunctionName references
replace colorTable and reverseColorTable references (JSON files also
should be changed)
@hkfb
Copy link
Collaborator

hkfb commented Oct 3, 2024

🎉 This issue has been resolved in version wsc-common@1.0.0 🎉

The release is available on GitHub release

@hkfb hkfb added the released label Oct 3, 2024
hkfb pushed a commit that referenced this pull request Oct 3, 2024
# [1.0.0](https://github.com/equinor/webviz-subsurface-components/compare/subsurface-viewer@0.30.17...subsurface-viewer@1.0.0) (2024-10-03)

### Features

* ColorMapFunction type in WellLogView ([#2249](#2249)) ([96bcab4](96bcab4)), closes [#2054](#2054) [#2259](#2259)

### BREAKING CHANGES

* - colorMapFunctions array replaces colorTables array to combine color
tables and functions in single array
- colorMapFunctionName and reverseColorMapFunctionName references
replace colorTable and reverseColorTable references (JSON files also
should be changed)
@hkfb
Copy link
Collaborator

hkfb commented Oct 3, 2024

🎉 This issue has been resolved in version subsurface-viewer@1.0.0 🎉

The release is available on GitHub release

hkfb pushed a commit that referenced this pull request Oct 3, 2024
# [2.0.0](https://github.com/equinor/webviz-subsurface-components/compare/well-log-viewer@1.13.11...well-log-viewer@2.0.0) (2024-10-03)

### Features

* ColorMapFunction type in WellLogView ([#2249](#2249)) ([96bcab4](96bcab4)), closes [#2054](#2054) [#2259](#2259)

### BREAKING CHANGES

* - colorMapFunctions array replaces colorTables array to combine color
tables and functions in single array
- colorMapFunctionName and reverseColorMapFunctionName references
replace colorTable and reverseColorTable references (JSON files also
should be changed)
@hkfb
Copy link
Collaborator

hkfb commented Oct 3, 2024

🎉 This issue has been resolved in version well-log-viewer@2.0.0 🎉

The release is available on GitHub release

@iWowik iWowik deleted the ColorFunctions branch October 3, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AspenTech Task owned by AspenTech enhancement New feature or request released well-log-viewer
Projects
None yet
4 participants