-
Notifications
You must be signed in to change notification settings - Fork 104
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
Refactor measured data #1316
Refactor measured data #1316
Conversation
While we're doing this, perhaps change the name to |
eeadfbb
to
9f0c9e4
Compare
I dont disagree that the name should change, but that is a pretty large API break, and |
a1094cd
to
1a6579b
Compare
The last commit (1a6579b) is more of a POC. Because it changes the behavior quite a lot, an alternate implementation is to create a new object instead with a new name, as the behavior is a bit different (on the new one, there is no in-place overwrite, everything returns a new object, while the old behavior was to overwrite the data, but keep the object). |
Sorry for this review coming in piece by piece. I'll try to get all the comments in! Overall I think it looks good though, and it absolutely makes a difference for the better. It's tricky to get around the exposed internal data structure of ert.. |
When running the integration tests I'm getting quite harsh aborts:
So the issue occurs when the test tries to create the I am not getting this error when I'm just running the tests normally in vscode, only in debug mode. This issue does not seem to occur in PyCharm. I am not getting the issue when using the |
Does it happen from the command line? Because it is before any of the production code changed in this PR is even touch I will argue that it is outside the scope of this PR, but perhaps make an issue? |
a58efa8
to
dfdc7ef
Compare
Yeah, I don't think this is a regression error, it's just surfacing due to the added test. It's a bit difficult to pin point what the problem is, as the only way I've reproduced the error is through debug the test with vscode.. |
22dd4bc
to
719d0bd
Compare
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.
Alright, looks good!
945fe3e
to
87a83ed
Compare
The loading of data was quite slow when several observation keys were pointing to the same response, as observations and response were read per keys. This changes to reading the data for all matching observations before merging the observations and response.
87a83ed
to
919692f
Compare
Issue
Resolves equinor/semeio#241, #1208