Code review comment for lp:~maddevelopers/mg5amcnlo/2.3.4-low-memory

Thanks,

>> 3) concerning the ncores options, would not be better to pass to the class the self.options of the interface? Such that we do not have to multiply the optional argument when we to have access to any of the options define in that dictionary (this should have been done for OLP)
> i don’t know… many of the interface options do not make sense for the amplitude generation, and will therefore have to be removed…

No just keep self.options and do not touch to it
such that this is just a pointer to the one of the interface
This will not use any memory (just a pointer in term of memory), actually less that what you do right now. (since you store two additional value if we count OLP)
So yes you will have access to pointless information but I do not see the problem.

Thanks a lot for the rest.

Cheers,

Olivier

> On Feb 19, 2016, at 09:33, marco zaro <email address hidden> wrote:
>
> Ciao Olivier,
> thanks for the review
> On 18 Feb 2016, at 20:44, Olivier Mattelaer <email address hidden> wrote:
>
>> Review: Needs Information
>>
>> Hi Marco,
>>
>> That's so cool, I missed the fact that a library was allowing to evade the GIL limitation.
>> This is so much nicer than using the Threading library (like the multi-core option of MG).
>> I guess that we should think to change the multi-core options of MG to move to this multiprocessing library.
>>
>>
>> 1) Is there any reason why this mode is not set as default?
> for simple processes, the old mode works OK, and has a more detailed logging (number of diagrams, etc).
>> 2) It should be indicated in the UpdateNotes how to use it.
> done
>> 3) concerning the ncores options, would not be better to pass to the class the self.options of the interface? Such that we do not have to multiply the optional argument when we to have access to any of the options define in that dictionary (this should have been done for OLP)
> i don’t know… many of the interface options do not make sense for the amplitude generation, and will therefore have to be removed…
>
>> 4)can you check why you set self['has_loops'] = False (line 466 of the diff) around line 220 of madgraph/fks/fks_helas_objects.py
> it starts with false, but then, after the calls to
> async_generate_born
> it is updated
> has_loops = bornout[2]
> self['has_loops'] = self['has_loops'] or has_loops
>
>> 6) Would not be better to not write the data on a file (via pickle), but add them in a multiprocessing.queue object that can be used by the main process? Or do you see a problem with that?
> The main reason to use pkl files is to keep the RAM as free as possible
>> 7) in madgraph/interface/amcatnlo_interface.py you define glob_directories_map
>> Defining a non static global is always annoying and creates troubles very often ( I have to apologize to have done it in aloha). Is it really necessary? and is it thread safe?
> It can be put as global. Josh said that this is related to memory/cpu performance. I think it is thread safe
>> 8) Is it possible to have a printout when the last process is generated and that you collect the info (I guess that you do that) since it hangs on one core on a line of type:
>> INFO: Generating born process: s~ s > t t t~ t~ [ all = QCD ]
>> I guess he is doing something else actually.
> i have added some logging
>>
>> So this is very very nice. Nice work!!
>>
> I have pushed the changes, now I’ll look at valentin’s comments…
>
> Cheers,
>
> Marco
>
>> Cheers,
>>
>> Olivier
>>
>>
>> --
>> 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
> You are reviewing the proposed merge of lp:~maddevelopers/mg5amcnlo/2.3.4-low-memory into lp:~maddevelopers/mg5amcnlo/2.3.4.