Fix for logL_birth

[williamjameshandley]

Description

This PR fixes #310 by adding an explicit guard for logL_birth, dropping nans and infs before plotting

It's not the neastest solution, so any suggestions for improvements are welcome.

Checklist:

• I have performed a self-review of my own code
• My code is PEP8 compliant (flake8 anesthetic tests)
• My code contains compliant docstrings (pydocstyle --convention=numpy anesthetic)
• New and existing unit tests pass locally with my changes (python -m pytest)
• I have added tests that prove my fix is effective or that my feature works
• I have appropriately incremented the semantic version number in both README.rst and anesthetic/_version.py

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (16083c4) to head (7eb1869).

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #324   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines         3041      3052   +11     
=========================================
+ Hits          3041      3052   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lukashergt]

See comments inline.

[lukashergt]

• The .dropna() shouldn't be necessary.
• Shall we separate inf replacement and plotting to reduce code duplication? We could have

  if x == 'logL_birth':
      selfx = self[x].replace(...)
  else:
      selfx = self[x]
  selfx.plot(...)

• I feel like changing infs to nans shouldn't happen silently. Shall we at least have a warning that we are setting -inf to nan for plotting purposes?

[williamjameshandley]

dropna is necessary for the 2D KDE, but not otherwiseo so I've removed those.
I've reduced code repetition as suggested.
With regard to infs to nans, I'm not that keen to have a warning for default behaviour (i.e. samples.plot_1d())

[lukashergt]

With regard to infs to nans, I'm not that keen to have a warning for default behaviour (i.e. samples.plot_1d())

Well, I'd argue here that samples.plot_1d() or samples.plot_2d() is not default behaviour for nested sampling data (nor for anesthetic dataframes in general, which typically come with very many columns), otherwise this would have come up earlier, too. By default the user should specify which columns are to be plotted.

So I think a warning message could help the new user (who is most likely to call the plotting functions without specifying columns) to learn faster how to use the plotting command. This is also how this PR came to be: new user simply trying samples.plot_1d() to see if it works...

I agree that too many warning messages are annoying and get in the way, but how often do we intentionally plot the posterior distribution of logL_birth? Also, for people that indeed want to intentionally plot logL_birth, they can easily avoid the warning by dropping infs first.

[williamjameshandley]

I am persuaded by this, so think we should have a warning message.

If we're going down a warning route, should we restrict this to logL_birth ? The code would be much neater if we just moved the relevant inf guards into kde_contour_plot_2d and kde_1d and kde_2d. This would then be more consistent with the other functions behaviour, e.g. samples.plot(), which just ignores nans and infs.

[lukashergt]

should we restrict this to logL_birth ?

We could do either logL_birth specifically, or anything with infs generally. Happy with both, but the warning should say something along the lines "there are infs in columns [logL_birth, ...]".

The infs will also cause problems for histograms, so I think catching them in plot_1d and plot_2d is probably the right place...?

[yallup]

My input from stumbling across this discussion via #332

Perhaps the default behavior is sensible (include nlive logl and loglbirth in the plot if nothing is specified), but it strikes me that the easiest thing to make the various kde errors less impactful is add a kind=scatter and make that the default for the specific case that the columns to plot are empty?

[williamjameshandley]

Note that this has come up in the past in #96, which this PR obseletes, so that test is now removed.

[lukashergt]

The current question is whether we should change the default behaviour to ignoring the columns logL, logL_birth, and nlive by default?

I would vote against that default change. plot_1d and plot_2d are more general Samples methods which I don't think should drop any columns by default.

Those columns are more specific NestedSamples parameters. We could create NestedSamples.plot_1d and NestedSamples.plot_2d methods which would handle the treatment of those parameters (and then forward to Samples.plot_1d etc.), be it a warning or a change to default behaviour. I'd prefer a simple warning.

Perhaps the default behavior is sensible (include nlive logl and loglbirth in the plot if nothing is specified), but it strikes me that the easiest thing to make the various kde errors less impactful is add a kind=scatter and make that the default for the specific case that the columns to plot are empty?

I'd keep the kde default, but am open to be convinced otherwise.
Either way, it might be nice to have a simpler shortcut for scatter plots, so maybe we should create a kind='scatter' shortcut, which uses 'scatter_2d' in the lower triangle (and 'hist_1d' on the diagonal?)?

[williamjameshandley]

Either way, it might be nice to have a simpler shortcut for scatter plots, so maybe we should create a kind='scatter' shortcut, which uses 'scatter_2d' in the lower triangle (and 'hist_1d' on the diagonal?)?

Added scatter and scatter_2d to default kinds in b51b3ef

[williamjameshandley]

I would also vote to keep things in Samples and avoid a NestedSamples override.

[lukashergt]

@williamjameshandley, if you are happy with my tweaks, feel free to squash and merge.