-
Notifications
You must be signed in to change notification settings - Fork 107
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 plot_api return all observations for response #9087
Make plot_api return all observations for response #9087
Conversation
9ac354a
to
aaefcbb
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.
LGTM
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 ok as is but with some structure/style comments 🐎
src/ert/gui/tools/plot/plot_api.py
Outdated
|
||
observations = response.json() | ||
observations_dfs = [] | ||
|
||
if not response.json(): |
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.
Why not replace if not response.json():
with if not observations:
if you already declared observations = response.json()
on ln 209
src/ert/gui/tools/plot/plot_api.py
Outdated
if not response.json(): | ||
continue | ||
try: | ||
obs = response.json()[0] | ||
response.json()[0] |
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.
Similar why not replace response.json()[0]
with observations[0]
src/ert/gui/tools/plot/plot_api.py
Outdated
try: | ||
obs = response.json()[0] | ||
response.json()[0] | ||
except (KeyError, IndexError, JSONDecodeError) as e: | ||
raise httpx.RequestError( | ||
f"Observation schema might have changed key={key}, ensemble_name={ensemble.name}, e={e}" | ||
) from e |
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.
It is unclear to me what the purpose of this section is now.
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.
I'm not sure either, but am also not confident to just remove it here as part of this bugfix, although I think this entire module, PlotAPI/Dark storage could be simplified alot
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.
I agree with keeping it for now
1a724db
to
3a852ef
Compare
3a852ef
to
781d473
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9087 +/- ##
=======================================
Coverage 90.75% 90.75%
=======================================
Files 350 350
Lines 21778 21781 +3
=======================================
+ Hits 19764 19767 +3
Misses 2014 2014
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Issue
Resolves #9077
Tested on RGS for stable and bleeding:
Before (left is rendering only one instead of 3 observations):
After: