Remove LocalEnsemble.load_all_gen_kw_data() - #12015
Conversation
| ensemble_data = ensemble.load_scalars().to_pandas().set_index("realization") | ||
| ensemble_data.columns.name = None | ||
| ensemble_data.index.name = "Realization" | ||
| ensemble_data = ensemble_data.sort_index(axis=1) |
There was a problem hiding this comment.
This is more or less the same as what was in load_all_gen_kw_data(), but it will be removed as we rewrite csv_export to use polars instead of pandas. We should do this gradually and in increments.
There was a problem hiding this comment.
@xjules
I will create an issue for this rewrite pandas -> polars, but it won't be done in this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12015 +/- ##
==========================================
- Coverage 90.73% 90.72% -0.01%
==========================================
Files 432 432
Lines 28911 28902 -9
==========================================
- Hits 26231 26222 -9
Misses 2680 2680
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
59fb512 to
0acac2f
Compare
CodSpeed Performance ReportMerging #12015 will not alter performanceComparing Summary
|
0acac2f to
cc0f87a
Compare
| for iter_nr in range(2): | ||
| ensemble = experiment.get_ensemble_by_name(f"iter-{iter_nr}") | ||
| data.append(ensemble.load_all_gen_kw_data()) | ||
| ensemble_data = ensemble.load_scalars().to_pandas().set_index("realization") |
There was a problem hiding this comment.
do we need to have pandas here? Ie. maybe just update snapshot?
| assert ( | ||
| np.linalg.det(posterior.cov().to_numpy()) | ||
| <= np.linalg.det(prior.cov().to_numpy()) + 0.001 | ||
| np.linalg.det(posterior.to_pandas().cov().to_numpy()) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, but not for all columns.
There was a problem hiding this comment.
posterior.cov().to_numpy()
cov_matrix = np.cov(posterior.to_numpy(), rowvar=False)
|
|
||
| for i in range(ensemble.ensemble_size): | ||
| assert ( | ||
| f",{name},{gen_kw_data.iloc[i]['COEFFS:a']:.6f},{gen_kw_data.iloc[i]['COEFFS:b']:.6f},{gen_kw_data.iloc[i]['COEFFS:c']:.6f},{misfit_data.iloc[i]['MISFIT:POLY_OBS']:.6f},{misfit_data.iloc[i]['MISFIT:TOTAL']:.6f}" |
There was a problem hiding this comment.
you can do directly row in pl.Dataframe
row_gen = gen_kw_data.row(i, named=True)
{row_gen['COEFFS:a']:.6f}| # generalized variance for the parameters. | ||
| assert ( | ||
| 0 | ||
| < np.linalg.det(df_posterior.cov().to_numpy()) |
There was a problem hiding this comment.
the same as above pl.cov ....
| data = ensemble.load_scalars().to_pandas().set_index("realization") | ||
| data.columns.name = None | ||
| data.index.name = "Realization" | ||
| data = data.sort_index(axis=1) |
There was a problem hiding this comment.
Can we update snapshot instead?
There was a problem hiding this comment.
That will change behavior. I don't think we want to do that in this PR.
There was a problem hiding this comment.
Not sure that I understand. What do you mean by behaviour?
There was a problem hiding this comment.
If we have to update the snapshot, the behavior/format has been altered. I don't think that should be done in this refactor-pr
cc0f87a to
9157fc6
Compare
This commit removes the function, and replaces it with `LocalEnsemble.load_scalars()` as it gradually moves from pandas towards polars.
9157fc6 to
56a1a39
Compare
xjules
left a comment
There was a problem hiding this comment.
Nice job @jonathan-eq ! Can you create an issue to remove pandas from the tests further on?
|
Blocked by equinor/semeio#772 |
Issue
Resolves #11939
Approach
This commit removes the function, and replaces it with
LocalEnsemble.load_scalars()as it gradually moves from pandas towards polars.(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests')When applicable