Mix bands and replicas

[scarlehoff]

Uses #1605 and #1606

Since the hopscotch replicas are single PDFs but we want to compare them to the "true" NNPDF we need to somehow be able to see in a single plot the hopscotch PDFs and the NNPDF bands. This is what I've come up with:

You might notice there is no label for the replicas, cannot see how to make them cross-talk (the bands and the replicas) but I don't think we will want to have this in the core code anyway? Or do we?

I've also removed the "central replica" since it makes no sense for the hopscotchy ones https://vp.nnpdf.science/avVrWwjGTsOtYFh1n5j66A==/

[RoyStegeman]

On Friday people commented on the +1 appearing in the charm plot

[Zaharid]

Might make sense to make this an extra module if it is to be used (but let's see I'd say).

[RoyStegeman]

So we want to add a boolean flag indicating whether or not to plot a PDF as a band. i.e. so we can do something like this:

pdfs:
  - {id: "NNPDF40_nnlo_as_01180_1000", label: "4.0 NNLO (1000 replicas)", plot_bands: False}

[scarlehoff]

I think what @Zaharid had in mind is plot_band: [1, 4] and that will be read by action plot_pdf_mixed

[RoyStegeman]

Right, also fine for me

[RoyStegeman]

We should also think of what to do about replica0

[RoyStegeman]

Please have a look if this is acceptable to you.

[Zaharid]

Why are we moving these things, which were specific to the bands pdfs here?

[RoyStegeman]

Because MixMatchPDFPlotter inherits from both ReplicaPDFPlotter and BandPDFPlotter, so the way ReplicaPDFPlotter deals with labels and handles has also been changed to be the same as BandPDFPlotter.

[Zaharid]

I'd rather not go that route: For one we don't want the legend for the replicas to be ComposedHandler. I think that all should look much more like what we do for pdfs_noband, i.e.

nnpdf/validphys2/src/validphys/pdfplots.py

Line 441 in b08ec66

labels.append(pdf.label)

and not touch much the base class.

[RoyStegeman]

But this still uses the flstate handles and labels from BandPDFPlotter. The issue here is that ReplicaPDFPlotter has different instances for the handles and labels.

[Zaharid]

Would most things work by inheriting from BandPDFPlotter first?

[RoyStegeman]

Sure, all BandPDFPlotter and ReplicaPDFPlotter do is add a function on PDFPlotter to draw the figures and add the labels, where for ReplicaPDFPlotter the default matplotlib legend is used.

What I did so far is move the label generation from BandPDFPlotter to PDFPlotter and add the labels to flstate also for ReplicaPDFPlotter. Instead of moving the label generation to PDFPlotter it is indeed also possible to make ReplicaPDFPlotter inherit from BandPDFPlotter but in my mind that organisation makes less sense.

[RoyStegeman]

@Zaharid what would be your preferred solution? Feel free to change this PR yourself if that's easier than explaining.

[Zaharid]

I wonder if we should expand plot_pdfs do the same thing as plot_pdfs_mixed and remove the later?

[RoyStegeman]

I wonder if we should expand plot_pdfs do the same thing as plot_pdfs_mixed and remove the later?

It does the same if you don't specify pdfs_plot_replicas. I was thinking about this as well, though since we would still have the plot_pdfreplicas (which we can't get rid off since the mixed_ variant doesn't print replica0) I don't mind also keeping plot_pdfs for symmetry.

[scarlehoff]

To me it looks ok (just realized I cannot approve the PR because I am technically the author). I think for the scope of what we want this PR this is good. The changes it introduces are minimal and the new features are contained in their own functions/checks.

Could you check that doing a report with replicas doesn't change anything? (in principle it shouldn't but having an explicit example linked in the PR could be useful for future reference).

[RoyStegeman]

Just to be sure, do I remember correctly that this is waiting for @Zaharid to implement some changes?

[RoyStegeman]

@Zaharid do you have any idea when you'll look a this?

[scarlehoff]

Should we merge this? i.e, is this "merge-ready" from your point of view @RoyStegeman ?

[Zaharid]

@scarlehoff @RoyStegeman This PR is adding a plot that is used in very limited circumstances, but that makes implementing new subclasses measurably more difficult as demostrated in this bit. I am not too happy with that as per the existing discussion. I see no pressure to merge it.

[scarlehoff]

that makes implementing new subclasses measurably more difficult as demostrated in this bit

How does this bit demonstrate that?

[scarlehoff]

(also, the very limited circumstances are already in the arxiv, which is more than one can say of many of the other plots that we have)

[Zaharid]

Instead of specifying label in the plot call and letting mpl handle the construction of the legend automatically, one has to pass in manually the components that make it, and override the behaviour of ComposedHandler, which is there for some reason despite being specific to the band plots.

This complexity makes sense where it was, in the band plots, because we want a complicated legend handler in there, but not in all present and future sublcasses.

[scarlehoff]

The only other option that I see is to copy the whole of the draw method of ReplicaPDFPlotter into MixMatchPDFPlotter instead of reusing ReplicaPDFPlotter.

[RoyStegeman]

Ok I think now it should be ok for @Zaharid ?

[Zaharid]

Looks good indeed. Everything works and the error messages are of the expected high quality

 $ validphys mixbandreplicas.yaml                             (nnpdfdev) 
[WARNING]: Output folder exists: /home/zah/nngit/nnpdf/validphys2/examples/output Overwriting contents
[ERROR]: Cannot process a resource:
Could not process the resource 'plot_pdfs_mixed', required by:
 - template_text
 - report
pdfs_plot_replicas should be a list of PDF IDs (strings) or a list of PDF indexes (integers, starting from one)

However I think we need to change the name of the parameter. Calling it pdfs_plot_replicas when we have plot_pdfreplicas to plot replicas is just too confusing.

I am happy to change the name myself if someone suggests a good one.

[RoyStegeman]

Good point. How about something along the lines of mixmatch_as_replicas? Since the class is called MixMatchPDFPlotter (actually, I'm not so sure what the "match" is doing in that name).

[scarlehoff]

Probably it makes sense to change to MixBandPDFPlotter

[RoyStegeman]

Feel free to update it again if you have a better idea @Zaharid.

[Zaharid]

I don't love that name either but can't really think of much.