Replaced dropdowns with VectorSelector

[rubenthoms]

Replaced dcc.DropDown with VectorSelector for selecting vectors in reservoir simulation timeseries plugin.

[HansKallekleiv]

Some comments based from testing the plugin interactively on the Reek testdata.
The selector looks good and intuitive to use! A real improvement compared to the dropdown layout.
I encountered two issues that should be fixed:

• Initial vectors specified in the config file is correctly showing in the VectorSelector, but is not rendered in the plotly graph.
• Atleast one vector is rendered as subplots 3 times.
  

Also, the changes here needs an update to the example config

[anders-kiaer]

LGTM, I only have a couple of minor comments. Since this touches AVG_, INTVL_ and also a plugin which is used quite a lot, I summon @asnyv for more 👀 before merge.

[asnyv]

A wild @asnyv has appeared!

Not sure if you have tested to throw in some csv-input with more or less random column names + checked how so called "UDQs" work? I'll try to do some testing on my side with various inputs to see how things work out. As @anders-kiaer mentions, it is a very frequently used plugin, so wise to spend some time on QC 🧐

[asnyv]

is this breaking change really necessary? Easy to wrap to existing vector1, vector2, and vector3 into a list in the code instead (checking if they are defined of course). I agree that it is cleaner, and probably better, but I think it isn't worth it to break existing setups for this.

[rubenthoms]

The existing setups are still working and we are showing a deprecation warning. I just wanted to remove it from the docs since it is going to be deprecated, i.e. users new to this plugin should rather stick to the new way of defining vectors. Does that make sense?

[asnyv]

I see that it is deprecated, I just don't think it is worth it to deprecate it (especially considering we are not making other changes on the input side as far as I know).
I don't see the point of adding the noise with a deprecation warning (and thus possible questions from users). It is slightly cleaner from the code side of course, but don't underestimate the extra issues you introduce on the user side due to the bunch of existing setups.

We will probably change other parts of the input for this plugin at a later stage, and considering users don't like to do more changes / updates to their setups than absolutely necessary, I suggest that we delay this deprecation to a time when we make a larger change to the plugin's input.

[rubenthoms]

@asnyv I see your point. Well, I am happy with either solution (we would just need to remove the deprecation warning and add back the old doc lines). Your call. What do you think, @anders-kiaer?

[anders-kiaer]

Lets revert to current input (vector{1,2,3}). I like the suggestion to change to list as input, so created #677 and we can take that one when refactoring the ReservoirSimulationTimeSeries plugin.

[rubenthoms]

@asnyv @anders-kiaer hmm, I have to admit that I found this plugin to be rather hard to read/understand/maintain. I heard that it was written/extended by several people who were new to the project. If this plugin is one of the most frequently used/important ones, it might be worth it spending some time on refactoring the code/code structure to make it better readable/understandable?

[asnyv]

@rubenthoms Yes, this plugin has been touched by a lot of hands. It is also among the older ones, so the style of newer plugins probably differ a bit. And there are still many features that could/should be included. So I can fully agree that a rewrite would be useful, but before anyone starts a major rewrite, I think we should make a better scope of which features that are needed and how it should look a bit more long term.

In the meantime: I think as mentioned that we should be careful with changing the user input without good reasons. Better for users to make one (or a few) big changes, than having to frequently update details.

[asnyv]

Overall it seems good

A few comments:

• The likely UDQs are something we should be able to get into the likely correct group without them being defined in the original list (there is a risk that we make mistakes, but probably worth it). That is mainly vectors with a base name up to 8 letters, and where the second character is U (There are more conditions that need to be met, we can discuss the details to limit the number of false positives).
• Probably something that has to be fixed on the JS side, but at least: I think we should limit to one tag per line, and that when a tag is defined you jump down one step. The reason is that when you click the arrows to iterate, the lengths might differ, and sometimes it jumps up and down between lines depending on length of vector, which is annoying.
• This is an issue also in our existing setup, but this is a nice time to fix it :) if no vector description exists, I think the tag should only by e.g. "vec_a" and not "vec_a - vec_a" as it is here (current implementation gives "vec_a (vec_a)".

what we do with the AVG_ and INTVL_ ones that Anders refer to, I am not sure. Think we should have a quick chat on that @anders-kiaer

[rubenthoms]

@asnyv @anders-kiaer Regarding your second point, Asgeir, I created a new PR in both wcc (equinor/webviz-core-components#148) and wsc (equinor/webviz-subsurface-components#405) which is making the new property lineBreakAfterTag (boolean) available (defaults to false).