Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make corrXY work with multiple GEN_KWs #9426
base: main
Are you sure you want to change the base?
Make corrXY work with multiple GEN_KWs #9426
Changes from all commits
5ad84b8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use the index for the observations here instead? I guess it is ok for summary observations, though perhaps a bit confusing as this number will not align with
report_step
, but forGEN_OBS
which already have an int index it would probably not align with that, and could be confusing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. Do I remember correctly that the index is a time-stamp or date for summary observations? We could perhaps still use it but perhaps a bit messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, was thinking a bit more about this, and should we consider doing it like we do for the snapshot in the gui showing the scaling factors when doing auto scaling? Those events are automatically written to csv by the client, so will be available to the user.
Something like:
ert/src/ert/analysis/_es_update.py
Line 239 in 7127528
ert/src/ert/analysis/_es_update.py
Line 252 in 7127528
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, these are the two types of indexes we have:
Are you suggesting we use the second part of the string as an index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, or perhaps send the whole thing as an event instead of saving it in storage. Then everyone gets the benefit right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the event thing. Would we then still be able to analyse the data from a Jupyter Notebook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit confusing if we just merge with existing data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you find confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is something on disk, we merge with that? Couldnt that potentially be done with a different threshold, and so no longer valid? I might be misunderstanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
self.mount_point
points to a specific ensemble. Can we run the same ensemble with different thresholds?Large diffs are not rendered by default.
Large diffs are not rendered by default.