-
Notifications
You must be signed in to change notification settings - Fork 117
Remove records concept for parameters & responses #10941
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
Conversation
dc8d853
to
e648a5c
Compare
815dc92
to
febf012
Compare
82c8ce6
to
4d8d655
Compare
3885865
to
30e88fa
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.
Looks like a nice improvement! I have some questions, but they might not be introduced in this PR, and as such could be fixed in follow up issues.
5c4c839
to
1c5d629
Compare
b4a931b
to
4b7d2d9
Compare
Should only be omitted until they can be handled by plotter
4b7d2d9
to
19e2334
Compare
CodSpeed Performance ReportMerging #10941 will improve performances by ×6Comparing Summary
Benchmarks breakdown
|
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.
LGTM! One minor comment
assert response_key is not None | ||
assert response_type is not None |
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.
Can they actually be None? Or is the typing wrong?
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.
Suggest deferring to separate issue to remove the None
typing.
Webviz-ert fails when it is removed, giving this:
File "webviz-ert/webviz_ert/data_loader/__init__.py", line 307, in compute_misfit
response_key, report_step = response_name.split("@", 1)
^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: not enough values to unpack (expected 2, got 1)
, and these tests fail:
test_data_for_response_doesnt_mistake_history_for_response
test_plot_api_handles_urlescape

Issue
Resolves #10684
Resolves #9059
Approach
Make separate endpoints for params and responses
PS: Will look into adding webviz ert plots into ert plotter to not maintain backward compat. The removals of some endpoints in this PR can be considered blocked by that.