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

Hi,
thanks to Olivier for the review and to Rik for the answers
On 02 Oct 2017, at 15:38, Rikkert Frederix <email address hidden> wrote:

> Hi Olivier,
>
> I see your points. However, most are very generally to FxFx and do not really apply specifically to this branch. Therefore, I'm not sure that this is the right time to implement them...
>
>>
>> 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)
> I think this would be a good idea. Marco, do you know if this is easy? Will at then also work with add-ons like MadSpin?

hmm…
currently the code searches a regular expression like [<option> = <orders>]. If option is not there (like for [QCD]) the option is set to ‘all’. In our case, [LOonly] would be equivalent to ‘all=LOonly’, which is certainly something that does not make sense. So, i don’t have any smarter idea than checking that, when one has orders=LOonly and option =all, then one assigns orders=QCD, option=LOonly with an ‘if’ around line 177 inside master_interface.
It is rather ugly, but I have no smarter idea…
What would you say?

Cheers,

Marco

>
>> 2) It would be great that ickkw being on default on 3 in this case.
>>
>
> Boahh.. don't know. Is this relevant? And how do you know when to put this? Do you suggest to count the 'j' in each 'add process' and check if it's more than in the 'generate' command? Is this sufficient?
>
>
>> 3) Would be great that the code crash if it is on 4.
>>
>
> Same as for previous comment.
>
>> 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)
>>
>
> The results for order=LO are not necessarily wrong. Therefore I would keep it. Same for fixed_order=ON.
>
>> 5) Does it make sense to have the shower ON by default? should not we have
>> turn it off?
>>
>
> It should be "On" in my opinion. But also here: in practice this will only be used by experimentalists. They know how to deal with either On or 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)
>>
>
> The [LOonly=QCD] option is not in beta. Rather the NLO+LO merging is. Hence at the level of the generation, one doesn't know yet if the user will use the merging or not.
>
>>
>> 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?
>
> No. Nor the cross section as the showering will/should reject a fraction of the events. Just as for NLO FxFx. However, I'm against removing this since it serves as a check that all required weights are written correctly in the event-file.
>
> Best,
> Rikkert
>
> --
> https://code.launchpad.net/~maddevelopers/mg5amcnlo/2.5.5_NLOandLOmerging/+merge/331151
> Your team MadDevelopers is subscribed to branch lp:~maddevelopers/mg5amcnlo/2.5.5_NLOandLOmerging.