equinor / webviz-subsurface

Webviz-config plugins for subsurface data.
https://github.com/orgs/equinor/projects/24
GNU General Public License v3.0
55 stars 58 forks source link

User loosing information of parameters due to stripping of string #1038

Open asnyv opened 2 years ago

asnyv commented 2 years ago

A user has reported issues with the parameter model, as the parts prior to (and including) the first : are stripped away. The user has a valid ERT setup where the parameters are named in a logical PARAMETER_TYPE:FORMATION format, e.g.:

PERMX:GARN PERMX:ILE PERMY:GARN PERMY:ILE

I would guess that this form of usage was the intention when implementing the structure in ERT originally, so I think it should be supported. I believe the stripping of parameters was introduced to reduce their length, but that is probably rather a user issue than something Webviz should resolve. I therefore suggest that we remove this stripping and rather encourage users to limit the length of parameter names, including the group name (or whatever that section is called)

https://github.com/equinor/webviz-subsurface/blob/8cebeb1ae60cdb5cfb7fe0e2a127c3ec609addf4/webviz_subsurface/_models/parameter_model.py#L110-L116

asnyv commented 2 years ago

@tnatt comments?

tnatt commented 2 years ago

Do I understand correct that the PERMX and PERMY is the prefix added by GEN_KW.. or is it the user that has named the parameter PERMX:GARN?

I guess it is the first one, and I actually like this stripping of the prefix from GEN_KW as they keep the parameter names not longer than they need to be, and I don't see the big need for that prefix information. If this were run through a design matrix you would not get that one anyway. In my opinion the parameter name should have included both the formation and the type.. how would you be able to understand what the parameter "GARN" represents if this was a design run?

But might be just me who prefer it this way😄 I am open to changing it, or at least make it an option! let's discuss in next standup 👍 maybe @lindjoha has a thought?

asnyv commented 2 years ago

Yes, the prefix is added by the group in GEN_KW. So I think it is actually a user who has used the functionality in ERT as it was intended (to structure the parameter names logically) 😉 The point of the chosen name was probably exactly as you say: to limit the length.

The parameters.json file is also structured the same way (one level for the "GEN_KW group" and sublevel for individual parameters), so it is probably a limitation of the designmatrix setup rather than misuse.

Since it is valid ERT syntax, we should at least support it. An alternative is to make a check and not remove the prefix if you get any non-unique parameter names.

lindjoha commented 2 years ago

I see the problem. They way we are normally using GEN_KW, we only have one or two groups and it's nice to get rid of the useless group names, f.ex GLOBAL_VAR. But I guess we should actually start using the group names properly instead. Then instead of GLOBAL_VAR:PERMMULT_TOFTE, the parameter should be named PERMMULT:TOFTE. Anyway, I agree with @asnyv that we need to support this, maybe removing the group name could be a config option?

And maybe this is another use case for the SmartAttributeSelector?

tnatt commented 2 years ago

yes exposing it as a config option would be a good idea @lindjoha 🙂 then the user can make the decision based on their set-up. Because I know many people do appreciate cleaner parameter names!

asnyv commented 2 years ago

I am not necessarily against config, but I also know that a lot of users find too much config options confusing, so think we should at least consider automation or standardization before we add another config-option 🙂