switch plotting code to makie

[SimonDanisch]

• port all recipes & tests
• documentation
• percy visual regression tests
• improved themeabiltity will do this in another PR

Visual changes from this PR (first image -> this PR, second image -> master)

All together 1

All together 2

Confusion matrix :Column

Confusion matrix :Row

Kappas IRA

Kappas no IRA

PR Curves

PRG curves

Reliability calibration

ROC Curves

[SimonDanisch]

Alright, a preview of the docs for the plotting functions is up at:
https://beacon-biosignals.github.io/Lighthouse.jl/previews/PR29/plotting/

@hannahilea, maybe you can push to this PR to fill in some more details about what those plotting functions do (if desired^^) ? Those details should go into the docstrings here:

Lighthouse.jl/src/plotting.jl

Line 348 in 470d7ef

plot_confusion_matrix(args...; kw...) = axisplot(plot_confusion_matrix!, args; kw...)

[hannahilea]

@hannahilea, maybe you can push to this PR to fill in some more details about what those plotting functions do (if desired^^)

Let's not block this PR on docs, since we already know those are not in great shape in this repo and adding them isn't top priority (yet).

[SimonDanisch]

I tried a few things to inline the plots somewhere, but it wasn't that easy without uploading the images anywhere...
So I guess for the comparison of Plots.jl + Makie.jl plots, one has to download this HTML file:
https://gist.github.com/SimonDanisch/1eec17096c9bef5637b103761c29edd1#file-plots_vs_makie-html

[ericphanson]

that comparison looks good! My thoughts:

• The text in the Makie ones seems a lot more legible, with the white text when the color is dark, and also on the bars in the barplots in a much more readable position.
• I think the legend in all_together_2 in the Makie would might work better as having 2 or 3 columns instead of 1, but that's kind of a minor thing (and it's definitely better than the weird empty axis in the Plots one!).
• most of the lines in the plots between the Plots vs Makie seem the same, but there are some differences in the numbers in the legends, like the AUC values in many of the plots, and in kappas_no_ira the "Multiclass" one is much larger for Makie (0.047 vs 0.011). Is that expected or is there a bug?

[SimonDanisch]

Hm, I let this run multiple times and got pretty different values every time, which is indeed surprising considering that the plots do look pretty much like a 1:1 match.
I also experimented with using StableRNGs to no avail.
This is the output after running ]test 2 times:

This will of course also make percy tests hard... @hannahilea do you think we are missing resetting an RNG somewhere, or is the training and metrics output hard to make fully deterministic?

[hannahilea]

@hannahilea do you think we are missing resetting an RNG somewhere

Yep! We use the global rng in majority(...), which means we should reseed the global rng at the start of testing:

Random.seed!(0)

[SimonDanisch]

I checked that one explicitly and it looks like we use a local RNG in the tests:
https://github.com/beacon-biosignals/Lighthouse.jl/blob/sd/makie/test/learn.jl#L93

[hannahilea]

majority(...) is called by the default elected param in learn(...), so tests that call learn(...) without passing in a non-default elected arg end up using that default rng. Looks like that is happening at least here [1], hence needing the global rng reseeded.

[1]

Lighthouse.jl/test/learn.jl

Line 47 in 66cbc09

Lighthouse.learn!(model, logger, () -> train_batches, () -> test_batches, votes;

[SimonDanisch]

Oh ha, of course you're right :) I thought I looked at all call sites of majority and made sure RNGs get passed through...

[hannahilea]

Yay :)

[hannahilea]

It's an easy one to miss...I was thrown by some nondeterminism in the LighthouseFlux tests that was due to this callsite, so I'm well familiar with it 😅

[hannahilea]

Ahhhh Simon this is looking so good! :D

Looking at the side-by-side plots, here are a few things I think need tweaking before we're good to go:

• kappas_ira.png I think the plot classes are inverted---the labels vs the colors are switched, so previously the grey bars were the expert-expert and now they are the algorithm-vs-expert.
• kappas_ira.png Could we make sure that the expert-expert bars are above the expert-algorithm bars (as in the original version)? This is intentional, to match the data formatting of publications that we want to easily compare against.
• kappas_ira.png The title should be the same as the old title
• kappas_ira.png Looks like we've lost the grid/minor x ticks; probably want those back pending intentional design choices around internal styling.
• reliability_calibration.png Would it be easy to make the scatter plot point outlines be the color of the line rather than black? The black is a little distracting.
• all_together.png/all_together_2.png The titles of some of the component plots are centered rather than left-justified; they were originally left-justified to make it easier to differentiate from x-axis labels.
• Missing plot: binary_discrimination_calibration_curves Looks like this one just doesn't exist as a solo comparison plot or in the all_together.png plots

Also, style improvements that can absolutely wait until future PRs, unless they're easy and you want to:

• all individual plots (ROC, PR, calibration, confusion) would ideally be square

Also, if at all possible, would be nice to have the side-by-side comparison images in this PR directly, rather than a link-out, so that we don't lose the history if you delete your gist at some point, and so that users can see the changes in this PR to know what they're getting when they bump versions. Makes sense to wait until it's the final version, though, if you prefer!

[hannahilea]

rng doesn't broadcast on 1.3

So I think if you wanted you could instead bump the required Julia version to 1.6. We're already going to be bumping the Lighthouse minor version, and I don't think any internal Beacon users are using <1.6 at this point.... Your call!

[SimonDanisch]

• The missing decorations in kappas were intentional to remove noise, considering that we already have the labels. I added them back for now, but didn't update the x ticks yet (so its still just 0 + 1).

I will try to figure out how to best upload a report here, so its easier to view... Its somehow surprisingly hard to just upload an html file somewhere and be able to directly look at it.

[SimonDanisch]

Alright, new comparison: https://serene-jang-51d2d2.netlify.app/
Note, sharpness comparisons aren't very useful with this, since those are PNGs and they're not shown at their native resolution...
Before we merge, I can collect the comparison into the PR description, so that one can see the differences right away ;)

[hannahilea]

Barring the one tiny suggested change, this looks good to go!!

Before we merge, I can collect the comparison into the PR description, so that one can see the differences right away ;)

Sounds good to me!

[ericphanson]

looks great!