handley-lab / anesthetic

Nested Sampling post-processing and plotting
https://anesthetic.readthedocs.io
MIT License
56 stars 16 forks source link

`ax.scatter` or `ax.plot` in `scatter_plot_2d`? #252

Open lukashergt opened 1 year ago

lukashergt commented 1 year ago

In https://github.com/williamjameshandley/anesthetic/pull/50#issue-479310788 we changed scatter_plot_2d such that it uses ax.plot instead of ax.scatter:

One additional change that is mixed in here is to revert ax.scatter to ax.plot in scatter_plot_2d, since I have observed scatter to make axis limits misbehave in non-trivial ways when applied to practical examples where data have very different scales. This has been documented and will be fixed in matplotlib v3.2, but until that is merged, we should revert to ax.plot. Whilst ax.scatter is more consistent with the name of the function, moving away from that actually neatens up a lot of the colouring issues that we have seen before (#32 #21 #45 #19 #31)

The bugs are supposedly fixed by now. Do we want to go back to ax.scatter? If so, 2.0.0 would be the time to do this.

Apparently ax.plot might be faster, but ax.scatter can potentially do more.

For reference:

AdamOrmondroyd commented 1 year ago

Unfortunately, pandas's ScatterPlot doesn't use the _make_plot() which calls _plot() pattern of HistPlot/KdePlot, but if I can get them to change this then I would agree with switching to ax.scatter

williamjameshandley commented 1 year ago

That's pretty clear. Let's remove this from 2.0.0, but keep the issue as a placeholder to remind us if/when @Ormorod fixes pandas.

AdamOrmondroyd commented 1 year ago

I hadn't realised you were keen for this change, I'll get a PR in to pandas asap

AdamOrmondroyd commented 1 year ago

If you could give pandas PR#51582 some love to get the ball rolling that would be great, as I suspect this will be harder to get through than a genuine bug fix