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

Refactor measured data #1316

Merged
merged 6 commits into from
Feb 16, 2021
Merged

Refactor measured data #1316

merged 6 commits into from
Feb 16, 2021

Conversation

oyvindeide
Copy link
Collaborator

Issue
Resolves equinor/semeio#241, #1208

@oyvindeide oyvindeide self-assigned this Jan 27, 2021
@oyvindeide oyvindeide marked this pull request as draft January 27, 2021 09:18
@pinkwah
Copy link
Contributor

pinkwah commented Jan 28, 2021

While we're doing this, perhaps change the name to ResponseData?

@oyvindeide oyvindeide force-pushed the refactor_measured branch 8 times, most recently from eeadfbb to 9f0c9e4 Compare February 4, 2021 09:07
@oyvindeide
Copy link
Collaborator Author

While we're doing this, perhaps change the name to ResponseData?

I dont disagree that the name should change, but that is a pretty large API break, and ResponseData is probably not descriptive enough as it also includes observations?

@oyvindeide oyvindeide marked this pull request as ready for review February 4, 2021 09:45
@oyvindeide oyvindeide force-pushed the refactor_measured branch 3 times, most recently from a1094cd to 1a6579b Compare February 4, 2021 13:34
@oyvindeide
Copy link
Collaborator Author

oyvindeide commented Feb 4, 2021 •
edited
Loading

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).

@lars-petter-hauge
Copy link
Contributor

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..

@wouterjdb wouterjdb mentioned this pull request Feb 9, 2021
Draft
11 tasks
@lars-petter-hauge
Copy link
Contributor

When running the integration tests I'm getting quite harsh aborts:

Fatal Python error: Aborted

Thread 0x000070000d424000 (most recent call first):
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 306 in wait
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 558 in wait
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/pydevd.py", line 232 in _on_run
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_daemon_thread.py", line 46 in run
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 932 in _bootstrap_inner
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 890 in _bootstrap

Thread 0x000070000c421000 (most recent call first):
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 306 in wait
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 558 in wait
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/pydevd.py", line 186 in _on_run
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_daemon_thread.py", line 46 in run
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 932 in _bootstrap_inner
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 890 in _bootstrap
....
Current thread 0x000000010be7bdc0 (most recent call first):
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/cwrap/prototype.py", line 218 in __call__
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/res/enkf/site_config.py", line 74 in __init__
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/res/enkf/res_config.py", line 192 in _alloc_from_content
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/res/enkf/res_config.py", line 127 in __init__
  File "/Users/LPHA/GitProjects/ert/tests/data/test_integration.py", line 27 in test_history_obs

So the issue occurs when the test tries to create the ResConfig object, more specifically the site_config.

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 pdb directly. So not a blocker I suppose.

@oyvindeide
Copy link
Collaborator Author

When running the integration tests I'm getting quite harsh aborts:

Fatal Python error: Aborted

Thread 0x000070000d424000 (most recent call first):
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 306 in wait
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 558 in wait
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/pydevd.py", line 232 in _on_run
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_daemon_thread.py", line 46 in run
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 932 in _bootstrap_inner
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 890 in _bootstrap

Thread 0x000070000c421000 (most recent call first):
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 306 in wait
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 558 in wait
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/pydevd.py", line 186 in _on_run
  File "/Users/LPHA/.vscode/extensions/ms-python.python-2021.1.502429796/pythonFiles/lib/python/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_daemon_thread.py", line 46 in run
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 932 in _bootstrap_inner
  File "/usr/local/Cellar/[email protected]/3.8.7_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/threading.py", line 890 in _bootstrap
....
Current thread 0x000000010be7bdc0 (most recent call first):
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/cwrap/prototype.py", line 218 in __call__
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/res/enkf/site_config.py", line 74 in __init__
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/res/enkf/res_config.py", line 192 in _alloc_from_content
  File "/Users/LPHA/venvs/ert_env/lib/python3.8/site-packages/res/enkf/res_config.py", line 127 in __init__
  File "/Users/LPHA/GitProjects/ert/tests/data/test_integration.py", line 27 in test_history_obs

So the issue occurs when the test tries to create the ResConfig object, more specifically the site_config.

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 pdb directly. So not a blocker I suppose.

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?

@lars-petter-hauge
Copy link
Contributor

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?

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..

@oyvindeide oyvindeide force-pushed the refactor_measured branch 7 times, most recently from 22dd4bc to 719d0bd Compare February 12, 2021 15:58
lars-petter-hauge
lars-petter-hauge approved these changes Feb 15, 2021
View reviewed changes
Copy link
Contributor

@lars-petter-hauge lars-petter-hauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, looks good!

@oyvindeide oyvindeide mentioned this pull request Feb 15, 2021
Merged
@oyvindeide oyvindeide force-pushed the refactor_measured branch 2 times, most recently from 945fe3e to 87a83ed Compare February 15, 2021 14:49
@oyvindeide
Refactor data loading
5d5a89a 
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.
@oyvindeide
Use numpy_vector instead of deprecated method
aabb93d
@oyvindeide
Fix snake_oil_field ert config
b051980
@oyvindeide
Depecate loading functions
89998e4
@oyvindeide
Refactor plot api
4d0a89a
@oyvindeide
Black plot api
919692f
@oyvindeide oyvindeide merged commit 5a1db7f into equinor:master Feb 16, 2021
@oyvindeide oyvindeide deleted the refactor_measured branch February 16, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lars-petter-hauge lars-petter-hauge lars-petter-hauge approved these changes

Assignees

@oyvindeide oyvindeide

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MPP] Running slow with large number of observations
3 participants
@oyvindeide @pinkwah @lars-petter-hauge