Use matplotlib method for bin creation for histogram - #13961
Conversation
|
From visual tests it seems like it should be correct. Would love input on this @berland |
41b0dfc to
274ee22
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13961 +/- ##
==========================================
+ Coverage 91.34% 91.38% +0.03%
==========================================
Files 477 482 +5
Lines 34062 34180 +118
==========================================
+ Hits 31115 31236 +121
+ Misses 2947 2944 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
274ee22 to
4c559c6
Compare
There was a problem hiding this comment.
Think the initial is wrong
There was a problem hiding this comment.
Pull request overview
This PR updates ERT’s GUI histogram plotting to use Matplotlib/NumPy’s built-in bin selection (bins="sqrt") rather than computing a single bin count shared across ensembles, addressing the reported issue where an ensemble’s histogram changed when other ensembles were toggled.
Changes:
- Remove global
bin_countcalculation based on the maximum ensemble size and delegate bin selection to Matplotlib (bins="sqrt"). - Rework log-scale bin edge generation using
np.histogram_bin_edgeson log-transformed values, and remove the custom_histogramLogBinshelper. - Update unit test expectations to match the new
_plotHistogramcall signature.
Reviewed changes
Copilot reviewed 2 out of 7 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ert/gui/plotting/ert_plots/histogram.py |
Switch histogram bin creation to Matplotlib/NumPy’s "sqrt" rule; remove shared bin-count logic and the custom log-binning helper. |
tests/ert/unit_tests/gui/plottery/test_histogram.py |
Adjust test mock call expectations after removing the bin_count argument. |
| mock_plot_histogram.assert_called_once_with( | ||
| ANY, | ||
| ANY, | ||
| ANY, | ||
| ANY, | ||
| use_log_scale=ANY, |
|
Looking at the image diff for e.g. docs/ert/getting_started/configuration/poly_new/with_observations/coeff_b.png it is clear that the bin sizes / bin count is changed for iter-1, even though I cannot determine that from the code that it should have done so. This obscures visualization of what the bug entailed in this PR. I wonder if we should ensure we fully understand what was wrong with the original code. Maybe by having a branch where both ways of computing bin edges are performed and printed to the terminal for qc, maybe also along with the data. Even better if the we could extract the old bin-making code to a function and prove it is wrong with a regression test, then fix it. |
4c559c6 to
a2c0330
Compare
Ensure correct count and fixes error that displayed faulty counts, when plotting multiple ensembles
a2c0330 to
edddc2f
Compare
|
Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#44 |
Issue
Resolves #13958
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests')When applicable