Code review comment for lp:~maddevelopers/mg5amcnlo/improved_scale_pdf_handling

Hi Rik,

This is a nice improvement. Here are my comments on the functionalities:

a) It would be nice to be able to define the list with a python syntax like
    range(10)
    [1,2,3]
    [a for a in range(100) if a%2!=0]
   This can simply be done with an 'eval' statement, and Olivier agreed to do it.
   So I just wrote that here for reference.

b) The default entry and text for the parameter 'reweight_PDF' and 'reweight_scale' is not very clear, especially because in fact they should now read ('consider_PDF_variation' and 'consider_scale_variation') but we cannot afford to change the name.
Also the stressing the fact that it should be a list *of booleans* should help.
So all in all, I simply suggest to change it by stressing 'booleans' and using the keyword 'variations' instead of 'dependence' which I think is clearer for most people (but I might be wrong).

True = reweight_scale ! Reweight to get scale variations using the
             ! rw_rscale and rw_fscale factors. Should be a list of booleans
             ! of length equal to that of the parameter 'dynamical_scale_choice'
             ! specifying which scales to include in the variation.

and similarly for 'reweight_PDF'.

Also, how can one pick several fixed scales? Is there no other way than modifying the code so as to add additional 'fake dynamical' scales which always return a constant one?

c) When using built-in pdfs, if I have a list defined for lhaid and reweight_PDF (with all reweight_PDF set to False) then the code crashes, which is bit annoying.

d) When doing a fixed order analysis, with two PDF sets and only the second of which has the PDF variations computed, then the plotting fails with:

RuntimeError : Error in LHAPDF::PDFSet::uncertainty. Input vector must contain values for all PDF members.
Please report this bug on https://bugs.launchpad.net/madgraph5
More information is found in '/Users/valentin/Documents/Work/MG5/improved_scale_pdf_handling/PROCNLO_loop_sm_0/run_06_tag_1_debug.log'.
Please attach this file to your report.

d) Otherwise the various runs I did went through fine and I really love the summary printout.
It is very clean. So now that you seem to have streamlined the mapping of the lhaid to the pdfset name, would it be considerable to be able to input the pdfset name instead of its lhaid in the run_card? (just an idea, of course not worth it if not trivial to do).

e) In the context of the PY8 merging systematics branch we talked with Stefan and thought it would be nice to add the 'name' attribute to the XML tag 'weightid' which would dictate the name that this weight will have in the hepmc file (following v3 standards).
We could consider to take the opportunity of this branch to add this here. If you agree, then we should follow the ongoing unofficial convention on how to name the weights of different PDF and scales so that analysis tools like Rivets an other can directly parse it.

Finally, the plots look good, nice work. However, the problem is that I have myself been working quite a lot on histograms.py in the PY8 branch (I used it to automatically generate the DJR plots for different merging scales as part of the systematics study. Btw, I also managed to automatically plot bands too, which was more of a pain than it sounds [but it can be turned off if needed]).
Skimming through your changes, I see that we are guaranteed to have considerable conflicts here, so I need some more time to think on how to do this properly. I might try to reach you on Skype at some point as it might help that we explain our structural changes to each other in live.
This histograms.py was supposed to be a quick thing waiting for MA5 to be properly interfaced to MG5aMC and with multiweight support, unfortunately I don't see this happening any time soon; that's too bad.

Anyway, this is very useful! Thanks for this.

Cheers