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: lithology track with pattern #1500

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

karbor
Copy link
Collaborator

@karbor karbor commented May 10, 2023

Added functionality to have discrete tracks with both color, pattern and name. Structured input a bit different, the new track type (lithologyTrack) has it's own spec (and not e.g. using color maps from a generic colormap input) as we see this as more intuitive when used.

@karbor karbor force-pushed the wellcorrelation_lithologytrack branch from b9bea1c to 4c6abc3 Compare May 10, 2023 11:39
@karbor karbor force-pushed the wellcorrelation_lithologytrack branch from 4c6abc3 to 41fd336 Compare May 10, 2023 11:48
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #1500 (fbb3a36) into master (4d1070a) will decrease coverage by 0.45%.
The diff coverage is 5.22%.

@@            Coverage Diff             @@
##           master    #1500      +/-   ##
==========================================
- Coverage   33.57%   33.13%   -0.45%     
==========================================
  Files         153      154       +1     
  Lines        8938     9067     +129     
  Branches     2390     2420      +30     
==========================================
+ Hits         3001     3004       +3     
- Misses       5906     6032     +126     
  Partials       31       31              
Impacted Files Coverage Δ
...src/lib/components/WellLogViewer/SyncLogViewer.tsx 37.98% <ø> (ø)
...omponents/WellLogViewer/components/WellLogView.tsx 40.29% <ø> (ø)
...onents/WellLogViewer/components/LithologyTrack.tsx 1.00% <1.00%> (ø)
...t/src/lib/components/WellLogViewer/utils/tracks.ts 44.75% <17.64%> (-1.70%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@karbor karbor force-pushed the wellcorrelation_lithologytrack branch from cc35618 to fbb3a36 Compare May 12, 2023 08:03
@karbor karbor marked this pull request as ready for review May 12, 2023 08:04
@hkfb
Copy link
Collaborator

hkfb commented May 12, 2023

@karbor there should already be support for color, label and patterns in the discrete plots. If you think the interface is unintuitive, then perhaps we could try to improve the interface? Adding duplicate code for roughly the same functionality will reduce maintainability of the code.

CC @Midtveit @mirisb @t0oF-azpn @Vladimir-Kokin

Snapshot for reference, with the new track on the right:
image

@karbor
Copy link
Collaborator Author

karbor commented May 24, 2023

As far as I could see from the storybook and understand from the code, patterns are only possible for wellpicks, i.e. they will cover the entire area across all tracks for a well. What we need is a track with that possibility - i.e. pattern(and later image), color and labels in a single track. If I'm wrong and this is possible in the existing code, could you provide an example?

@hkfb
Copy link
Collaborator

hkfb commented May 24, 2023

As far as I could see from the storybook and understand from the code, patterns are only possible for wellpicks, i.e. they will cover the entire area across all tracks for a well. What we need is a track with that possibility - i.e. pattern(and later image), color and labels in a single track. If I'm wrong and this is possible in the existing code, could you provide an example?

I see your point, and I agree it looks like your use case is not supported.

Is the only thing missing a mapping from discrete code / value to a pattern or image?

If so would it make sense to add to individual plots or tracks the pattern mapping shown in this example, ie. the patternsTable and patterns props? https://equinor.github.io/webviz-subsurface-components/storybook-static/?path=/story/welllogviewer-demo-synclogviewer--default

@Midtveit @Vladimir-Kokin

@karbor
Copy link
Collaborator Author

karbor commented May 24, 2023

I considered reusing the patternsTable/patterns from the existing interface also. Ended up with this as a proposal/start of maybe a more intuitive interface(and just keep it on the side of the existing to start with)? I am new to both webviz and front-end programming so I don't know it this is a good proposal for the interface. But we have seen working from the python-side with the component for a while, that there could be beneficial to look into if the interface to the component could be more intuitive and flexible. Maybe we could have more general discussion around that (in a meeting)?

@hkfb
Copy link
Collaborator

hkfb commented May 24, 2023

I considered reusing the patternsTable/patterns from the existing interface also. Ended up with this as a proposal/start of maybe a more intuitive interface(and just keep it on the side of the existing to start with)? I am new to both webviz and front-end programming so I don't know it this is a good proposal for the interface. But we have seen working from the python-side with the component for a while, that there could be beneficial to look into if the interface to the component could be more intuitive and flexible. Maybe we could have more general discussion around that (in a meeting)?

Would be good to have a discussion on the API design. I think there is already planned a discussion on the WellLogViewer around the end of the month @anders-kiaer although I'm on leave the next couple of weeks, so I probably won't attend.

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

Successfully merging this pull request may close these issues.

3 participants