Code review comment for lp:~maddevelopers/mg5amcnlo/2.5.5_NLOandLOmerging

Revision history for this message
Olivier Mattelaer (olivier-mattelaer) wrote :

Hi,

I'm starting to look at this. My question are so far on the interface/physics limitation.
Actually a lot of the point below should also be consider for FxFx run in general since they are not specific to LOonly.

1) the syntax "add process p p > e+ e- j [LOonly]" does not work. Would be great to not be force to do "add process p p > e+ e- j [LOonly=QCD]" since the QCD here is meaningless (I guess)

2) It would be great that ickkw being on default on 3 in this case.

3) Would be great that the code crash if it is on 4.

4) What would be the meaning of having order=LO in this setup? (This is actually a valid question even without the LOonly option). I guess the answer is None. Should we prevent it?
(actually same question with fixed_order=ON)

5) Does it make sense to have the shower ON by default? should not we have turn it off?

6) If this is to be consider as beta, I would prefer that a warning appear when the matrix-element are generated. (Such that this is clear)

7) When running, I have the following:
   --------------------------------------------------------------
      Summary:
      Process p p > e+ e- [QCD] ; p p > e+ e- j [LOonly=QCD]
      Run at p-p collider (6500.0 + 6500.0 GeV)
      Number of events generated: 10000
      Total cross section: 2.103e+03 +- 8.4e+00 pb
   --------------------------------------------------------------
      Scale variation (computed from LHE events):
          Dynamical_scale_choice -1 (envelope of 9 values):
              2.130e+03 pb +6.3% -11.9%
   --------------------------------------------------------------

Can we trust the scale variation?

I did not test the physics but I checked the code and this sounds fine.
I would have prefer that you factorise the code to have less a huge function with a bunch of if.
But this is not critical and we will be able to split the function in smaller piece if needed in the future.

Thanks for this work,

Olivier

« Back to merge proposal