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.

Sign up for GitHub

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

Jump to bottom

ParameterModel drops non-logarithmic parameters if one of the ensembles uses a logarithmic distribution #1221

Open
asnyv opened this issue May 24, 2023 · 6 comments · May be fixed by #1222
Open

ParameterModel drops non-logarithmic parameters if one of the ensembles uses a logarithmic distribution #1221

asnyv opened this issue May 24, 2023 · 6 comments · May be fixed by #1222
Labels
bug 🐛 Something isn't working

Comments

@asnyv
Copy link
Collaborator

asnyv commented May 24, 2023

The linked code drops the non-logarithmic column and renames the logarithmic ones. If someone is testing different distributions (logarithmic and non-logarithmic), the non-logarithmic distributions will be lost. I suggest that we short term skip dropping the non-logarithmic even though it creates some duplicates. Could be better to e.g. only keep the non-logarithmic which exists independent of chosen distribution, but instead have an option in the gui to use a logarithmic axis.

webviz-subsurface/webviz_subsurface/_models/parameter_model.py

Lines 112 to 119 in 1b1bb17

# Keep only LOG parameters
log_params = [param for param in self._dataframe if param.startswith("LOG10_")]
self._dataframe = self._dataframe.drop(
columns=[param.replace("LOG10_", "") for param in log_params]
)
self._dataframe = self._dataframe.rename(
columns={col: f"{col} (log)" for col in log_params}
)

@asnyv asnyv added the bug 🐛 Something isn't working label May 24, 2023
@asnyv asnyv added this to Webviz May 24, 2023
@github-project-automation github-project-automation bot moved this to Backlog 📝 in Webviz May 24, 2023
@asnyv
Copy link
Collaborator Author

asnyv commented May 24, 2023

any thoughts @lindjoha @tnatt @anders-kiaer @rnyb ?

@lindjoha
Copy link
Collaborator

I completely agree. It's better to keep both, and even better with the logarithmic axis option

ParameterModel is also a bit too smart when it removes the group name from the parameter name. Could we have some kind of parameter config options as input to the ParameterModel?

@asnyv
Copy link
Collaborator Author

asnyv commented May 24, 2023

Agree on the group name @lindjoha. Not sure about the config options: think we should ideally avoid more user config considering the structure of "next gen" webviz is likely going towards less manual user configuration?

@asnyv asnyv linked a pull request May 24, 2023 that will close this issue
Open
2 tasks
@tnatt
Copy link
Collaborator

tnatt commented May 26, 2023

agree on the logarithmic 👍 I don't like the long names we get with the GROUP though... looks messy 😄 but up to you.
I guess in the distant future when we start using the cloud based all this will be handled in a better manner 🤞

@tnatt
Copy link
Collaborator

tnatt commented May 26, 2023

splitting out the groups from the names and using them as filters would be better in my opinion.. but requires more work 🤔

@asnyv
Copy link
Collaborator Author

asnyv commented May 26, 2023

Suggestion:

  • We fix the log-issue for now as in my suggested PR. Can consider to release that as a bug fix as I know some users have faced the issue.
  • Create a new issue where we discuss the group stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug 🐛 Something isn't working
Projects
Webviz
Status: Backlog 📝
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Keep non-log parameters asnyv/webviz-subsurface
3 participants
@asnyv @lindjoha @tnatt