Ciao Valentin,
Thanks for the review…
I should have addressed all points now, see below…
cheers,

Marco
On 19 Feb 2016, at 21:04, Valentin Hirschi <email address hidden> wrote:

> Hi Marco,
> 
> One more thing, please check how you defined the possible values for the option 'low_mem_multicore_nlo_generation' that you have introduced because the auto completion to 'True False default' does not work.

fixed
> 
>> Ciao Valentin,
>> thanks for your review, so:
> 
>> On 18 Feb 2016, at 23:48, Valentin Hirschi <email address hidden> wrote:
>> 
>>> Review: Needs Fixing
>>> 
>>> Hi Marco,
>>> 
>>> Awesome job, really.
>>> 
>>> On the testing side, I noticed the following:
>>> 
>>> a) Doing a full comparison of p p > t t~ j [QCD] generated in single and
>> multicore, I noticed that the file 'SubProcesses/proc_characteristics' was
>> incorrect in mutlicore, since it contains:
>>> ninitial = 0
>>> nexternal = 0
>> hmm right, i was not checking this file with the unit test. I have added it to
>> the test, it should be fixed now (rev 315)
>>> 
>>> b) This implementation broke process generation with another OLP than
>> MadLoop (i.e. Gosam).
>>> When selecting Gosam as OLP, the generation crashes at:
>>> 
>>> FKSProcessError : Wrong type of argument for matrix_elements in function
>> write_lh_order.
>>> 
>>> from
>>> 
>>> [...]
>>> File "/Users/valentin/Documents/Work/MG5/2.3.4-low-
>> memory/madgraph/iolibs/export_fks.py", line 1355, in
>> generate_virtuals_from_OLP
>>>   self.write_lh_order(filename, FKSHMultiproc.get('matrix_elements'),OLP)
>>> File "/Users/valentin/Documents/Work/MG5/2.3.4-low-
>> memory/madgraph/iolibs/export_fks.py", line 1623, in write_lh_order
>>>   'matrix_elements in function write_lh_order.’)
>>> 
>>> Of course we don't want to have any sort of parallelization on behalf of
>> GoSam, but normally here the only thing that is done is the writing of the
>> leshouches order. The actual generation with GoSam should take place at the
>> very end, basically after that the process is generated and the
>> parallelization is done. So if this is not too difficult to fix, I think it
>> would be worth doing so. Otherwise, explicitly forbid this mode when OLP is
>> not MadLoop, but that would be weird because there is no reason for doing so.
>>> Note that you don't need to have GoSam installed to test this I think,
>> because it crashes when writing the leshouches order file, so before actually
>> calling GoSam.
>>> 
>> I have added a unit test also for this and reproduced your error. It should be
>> fixed now (rev 316), please check that i did not screw anything, as some
>> modifications to the code were needed (the unit tests pass after trival fixes)
> 
> So when not using the low-mem option, now OLP = GoSam crashes with:
> 
> AttributeError : 'MasterCmd' object has no attribute ‘born_processes_for_olp'

> 
> at 
> 
> [...]
>  File "/Users/valentin/Documents/Work/MG5/2.3.4-low-memory/madgraph/interface/amcatnlo_interface.py", line 557, in do_output
>    self.born_processes_for_olp,self._export_dir,self.options['OLP'])
> AttributeError: 'MasterCmd' object has no attribute ‘born_processes_for_olp’
that should be ok now, please check (I don’t have GoSam)
> 
> And when doing it with the new option turned on, it goes through but crashes at the compilation because apparently you still linking the version of 'BinothLHA.f' for MadLoop and not the one for GoSam as it is done when not using the new option.
also this (and also the real=QCD was not working correct) fixed

>> 
>>> c) This is a problem which was not introduced by this branch but that I
>> realized now and needs to be fixed. It showed up because I saw that you had to
>> play around with the overall 'max_loop_coefs' value for different
>> FKSHelasMatrixElements.
>>> So imagine you do the following:
>>> 
>>> MG5_aMC>import model HC_NLO_X0_UFO-heft
>>> MG5_aMC>generate g g > x0 [virt=QCD]
>>> MG5_aMC>add process g g > z [virt=QCD]
>>> MG5_aMC>output MyTest
>>> 
>>> Then you will see the message:
>>> """
>>> ML5 has just output processes which do not share the same maximum loop
>> wavefunction size or the  same maximum loop vertex rank. This is potentially
>> dangerous. Please prefer to output them separately.
>>> """
>>> 
>>> This is because if you look at the different coef_specs.inc, you will find:
>>> 
>>>> cat MyTest/SubProcesses/P0_gg_x0/coef_specs.inc
>>>     INTEGER MAXLWFSIZE
>>>     PARAMETER (MAXLWFSIZE=4)
>>>     INTEGER LOOP_MAXCOEFS
>>>     PARAMETER (LOOP_MAXCOEFS=70)
>>>     INTEGER VERTEXMAXCOEFS
>>>     PARAMETER (VERTEXMAXCOEFS=15)
>>> 
>>>> cat MyTest/SubProcesses/P1_gg_z/coef_specs.inc
>>>     INTEGER MAXLWFSIZE
>>>     PARAMETER (MAXLWFSIZE=4)
>>>     INTEGER LOOP_MAXCOEFS
>>>     PARAMETER (LOOP_MAXCOEFS=35)
>>>     INTEGER VERTEXMAXCOEFS
>>>     PARAMETER (VERTEXMAXCOEFS=5)
>>> 
>>> But then you see that in Source/DHELAS you have:
>>> 
>>>> cat MyTest/Source/DHELAS/coef_specs.inc
>>>     INTEGER MAXLWFSIZE
>>>     PARAMETER (MAXLWFSIZE=4)
>>>     INTEGER VERTEXMAXCOEFS
>>>     PARAMETER (VERTEXMAXCOEFS=15)
>>> 
>>> The tricky thing here is that DHELAS, which is used by both subprocesses,
>> depends on VERTEXMAXCOEFS which is different for both subprocesses.
>>> It is actually fine but *only if* the value written for MAXLWFSIZE and
>> VERTEXMAXCOEFS in 'Source/DHELAS/coef_specs.inc' are the *maximal value*
>> amongst all virtual subprocesses.
>>> 
>>> To insure this, in MadLoop standalone output I call (after the generation),
>> around line 540 of loop_interface.py, the function
>> 'self._curr_exporter.fix_coef_specs' which will take care to correct these two
>> values in coef_specs.inc of DHELAS so as to always be the maximal ones.
>>> 
>>> Could you also do this for the FKS output and make sure it works both in
>> sequential and parallel generation? Don't hesitate to ask my help for this if
>> needed.
>>> For the FKS output it can be tested simply with:
>>> 
>>> MG5_aMC>import model HC_NLO_X0_UFO-heft
>>> MG5_aMC>generate u u~ > z [QCD]
>>> MG5_aMC>add process g g > h [QCD]
>>> MG5_aMC>output MyTest
>>> 
>> I have checked this, both with and without the new NLO generation, and in the
>> latter case also inverting the order of the processes. The coef specs is
>> correct in all cases. You can have a look at the function defined in
>> export_fks around line 3298
> 
> Perfect, this works like a charm (tried all combinations of new/old mode and order of processes)
> 
>>> d) I took the liberty of fixing in your branch an unrelated issue that arose
>> when attempting an FKS output with the MG5aMC option 'loop_color_flows' set to
>> True.
> 
> I had not pushed this; I just did it now. So please make sure you pull the branch before merging it.
> 
>>> 
>>> Now about the python code changes themselves:
>>> 
>>> 1) I see the following lines (I know they were there before) in
>> fks_helas_objects.py:
>>> 
>>> self.matrix_element =
>> copy.deepcopy(real_me_list[real_amp_list.index(fksrealproc.amplitude)])
>>> self.matrix_element['processes'] =
>> copy.deepcopy(self.matrix_element['processes'])
>>> 
>>> Isn't it a bit dangerous to brute-force deepcopy the entire
>> HelasMatrixElement? It can easily unnecessarily increase the memory footprint,
>> for example the *entire model* is copied there whereas the idea was to always
>> have one instance of the model living in the framework memory at any time.
>> i think that was really needed i.e. the simple copy did not work
>>> It should be possible to do the copy "smartly" by hand instead of using
>> deepcopy, no?
>> hm right… anyway, this is for the old way of generating the code
> 
> Mhh ok, so you are saying that there is no deepcopy of matrix elements at all in the new mode?
> I think this is more than just esthetics and must be taken seriously because it can really lead to a huge increase in memory usage. But if this is now done only in the old mode, then it is fine indeed.
> 
>>> Also, the second deepcopy is mysterious to me, given the one preceding it.
>>> 
>>> 2) For these lines:
>>> 
>>> #only available after export has been done, so has to be returned from here
>>>   max_loop_vertex_rank = -99
>>>   if me.virt_matrix_element:
>>>       max_loop_vertex_rank =
>> me.virt_matrix_element.get_max_loop_vertex_rank()
>>> 
>>> This is the kind of lines I was referring to in my point c) above and for
>> which care is needed. We must make sure that they work corretly in the context
>> of c).
>>> 
>>> 3) This change in banner.py is curious
>>> 
>>> -            for leg in proc[0]['legs']:
>>> +            for leg in proc['legs’]:
>> 
>> it is related to a change in the arguments which are passed, ie processes was
>> a list of list of processes, now it is a list of processes (check the diffs in
>> export_fks around line 1180)
> 
> I understand, but I was just worried that this function RunCardNLO.create_default_for_process(... was also called at some other place in the code, still using the old format for the arguments passed and therefore crashed. But if you checked in the code for all the usage of this function and you are confident that there is no side effect, then it is fine of course.
> 
>> 
>> I think that is all…
>> thanks again for the review!
> 
> That is a really nice and important feature, so I'm glad to do my minimal share of work on it.
> 
> Cheers
> 
>> Cheers,
>> 
>> Marco
>> 
>>> 
>>> Was it a bug or did you change the structure of 'proc'?  If the latter is
>> true, then are you sure that now proc will always be list of processes in this
>> function?
>>> 
>>> That's all I could spot for now.
>>> 
>>> Thanks again for this! It is really great that you could pick up Josh's
>> work, polish/improve it and have it merged in the trunk. My hat's off to you.
>>> 
>>> Cheers
>>> --
>>> https://code.launchpad.net/~maddevelopers/mg5amcnlo/2.3.4-low-
>> memory/+merge/286337
>>> Your team MadDevelopers is requested to review the proposed merge of
>> lp:~maddevelopers/mg5amcnlo/2.3.4-low-memory into
>> lp:~maddevelopers/mg5amcnlo/2.3.4.
> -- 
> https://code.launchpad.net/~maddevelopers/mg5amcnlo/2.3.4-low-memory/+merge/286337
> Your team MadDevelopers is requested to review the proposed merge of lp:~maddevelopers/mg5amcnlo/2.3.4-low-memory into lp:~maddevelopers/mg5amcnlo/2.3.4.