tests: vertically centre heatmap #4

[dsgibbons]

tests.plots.test_heatmap was failing due to a mismatch between the expected and generated plots.

This PR make some minor tweaks to the heatmap function so that the colorbar is plotted correctly.

Related to #4.

[dsgibbons]

heatmap tests pass now

[thatlittleboy]

LGTM

[dsgibbons]

Any last thoughts @connortann?

[connortann]

I have a few rather minor comments:

1. Please rebase the PR on top of master, to check that Ruff passes ok. (Side note: we might want to agree a convention for how we complete PRs. How about "Squash & Merge"?)

2. There is a warning in the tests from the colorbar about the specification of axes, which I think can be fixed by passing the relevant axis as a keyword argument around _heatmap.py: L137 :

tests/plots/test_heatmap.py::test_heatmap
tests/plots/test_heatmap.py::test_heatmap_feature_order
  Unable to determine Axes to steal space for Colorbar. Using gca(), but will raise in the future. Either provide the *cax* argument to use as the Axes for the Colorbar, provide the *ax* argument to steal space from it, or add *mappable* to an Axes.

3. There is another warning in the tests about trying to display the figure, which I believe can be fixed by setting show=False in the call to heatmap():

tests/plots/test_heatmap.py::test_heatmap
tests/plots/test_heatmap.py::test_heatmap_feature_order
  Matplotlib is currently using agg, which is a non-GUI backend, so cannot show the figure.

4. We should ensure that matplotlib tests close the relevant figures after each test run, to avoid taking up memory and generating warnings. I'll make a separate PR to address that though. EDIT: Chore: Close matplotlib plots after use #40

[thatlittleboy]

I think Squash and Merge is good. Keeps the git log a little bit neater. (Whatever we go with, preferably @dsgibbons can also restrict it in the settings so that it is the only option we have when merging).

[dsgibbons]

Please rebase the PR on top of master, to check that Ruff passes ok. (Side note: we might want to agree a convention for how we complete PRs. How about "Squash & Merge"?)

Agreed.

I think Squash and Merge is good. Keeps the git log a little bit neater. (Whatever we go with, preferably @dsgibbons can also restrict it in the settings so that it is the only option we have when merging).

Done!

[dsgibbons]

@connortann

There is another warning in the tests about trying to display the figure, which I believe can be fixed by setting show=False in the call to heatmap():

Done.

There is a warning in the tests from the colorbar about the specification of axes, which I think can be fixed by passing the relevant axis as a keyword argument around _heatmap.py: L137 :

Some posts note that this warning has become an error for some users:

• beeswamplot value error shap/shap#2721
• Fix shap.summary_plot to work with matplotlib 3.6.0 shap/shap#2697

shap#2697 addresses this by adding pl.gca() to pl.colorbar(..., ax=pl.gca()). I'm not sure if this is the best fix, or if something like the proposal from #5 would be better long term (and aligns more with what @connortann suggested).

I think this should PR should be merged as is, and we can address the ax argument as a separate issue.