Him
> Hi,
> 
> a) why only showing the first line in 'INFO' and the rest in 'DEBUG' ? imho,
> the whole set of lines should be in INFO.
> The sandboxing, allow very low control on what is printed, so if a line do not
> start by INFO, it pass in DEBUG.

Ok, then maybe just print these lines all one by one so that they are prefixed by INFO and therefore correctly handled by the sandboxing? Anyway, up to you.

> 
> > b) Debug left at:
> > CRITICAL:  w_orig, w_new, w_new/w_orig =  0.393281380656 0.393281380656 1.0
> [reweight_interface.py at line 813]
> 
> Sorry, I was studying a weird behavior today and forget to remove that print
> statement.
> 
> > c) I tried to run the default PY8 analysis for p p > e+ ve [QCD] with your
> kamikaze reweighting 'ON', i.e.
> > by setting 'py8an_HwU_pp_lvl.o HwU.o' in the analysis parameter of the
> shower card, and it fails with:
> 
> 
> It runs fine with HW6. Never succeed to run PY8 analysis… so I will keep it
> like that.
> Paolo told me that the parsing should have be fine….

I confirm that an HW6 run or PYTHIA6PT run goes through fine, so this issue is really a problem of the parsing in the py8 driver/analysis which is not really part of this branch, so it will have to be fixed somewhere else I agree. But I think that it is important that it will; hopefully this will come for free when merging with the improved_scale/pdf branch.

> 
> > d) In the card, I think it would be better to use another example than:
> 
> This is fine like that
> 
> > e) I think it is important that any valid process syntax gets correctly
> parsed by the reweighting module.
> 
> The problem is that a lower case is applied… The parser is the one of
> madgraph.
> Will check why.
> But the first fix is to allow such syntax. (why the coupling order should be
> case sensitive)

I don't see why they shouldn't be case sensitive. Anyway, there should be no need for a lower_case() here no?

> 
> > I guess I am missing some processes, but it would be nice if the error was
> more explicit!
> 
> That is fine like that

Ok.
You didn't reply to the other issues I found when changing the model, processes and directory to reuse. So when doing exact NLO reweighting how are these process definition used? How do you reweight differently the Born and virtual in this case?

> 
> > f) The following syntax instruction in the reweight card is wrong:
> 
> Thanks. fixed
> 
> 
> > g) Can I ask you a favor? Could you add your fortran wrapper that includes
> all relevant MadLoop functions as well as the f2py meta-codes in the
> standalone MadLoop output as well? (and make sure there is a target in the
> makefile to f2py it). I find it very cool and it would be nice to have it
> available in the standalone output (doesn’t cost anything).
> 
> I guess you know the answer.

The answer is yes because it's just a simple 'cp command' to add to the standalone output? Great, thanks.

> 
> > h) Related to the point above, the reason why the intialization failed is
> because you have substituted check_sa.f !!!! So of course now it has not
> ‘main' program statement and when you try to compile it, you get :
> 
> Do not see what you are talking about.

We sorted that out on Skype, so it's ok.

Cheers

> Cheers,
> 
> Olivier
> 
> 
> > On Mar 3, 2016, at 00:15, Valentin Hirschi <email address hidden>
> wrote:
> >
> > Review: Needs Fixing
> >
> > Hi Olivier,
> >
> > Let me continue this review
> >
> > a) My OCD at work again, sorry:
> >>> NLO reweighting is on ON. Automatically set keep_rwgt_info to True -->
> detail: you could remove the 'on' ON.
> >>> remove leading space in ' When doing NLO reweighting, MG5aMC canno'
> >>> generating the square matrix element for reweighting -> generating squared
> matrix element for reweighting
> >>> REWEIGHT: starts to compute weight for events with the following
> modification to the param_card:
> > REWEIGHT: change model loop_qcd_qed_sm
> > DEBUG:  change process  p p > e+ ve
> > DEBUG:  change process  p p > e+ ve j
> >
> > why only showing the first line in 'INFO' and the rest in 'DEBUG' ? imho,
> the whole set of lines should be in INFO.
> >
> > b) Debug left at:
> > CRITICAL:  w_orig, w_new, w_new/w_orig =  0.393281380656 0.393281380656 1.0
> [reweight_interface.py at line 813]
> >
> > c) I tried to run the default PY8 analysis for p p > e+ ve [QCD] with your
> kamikaze reweighting 'ON', i.e.
> > by setting 'py8an_HwU_pp_lvl.o HwU.o' in the analysis parameter of the
> shower card, and it fails with:
> >
> > terminate called after throwing an instance of 'std::runtime_error'
> >  what():  Tried to read a file which does not start with the
> LesHouchesEvents tag.
> > Abort trap: 6
> >
> > This seems to come from the fact that the events are gzipped again before
> the shower step.
> > If I unzip them by hand and try again, it runs further but the
> shower+analysis stops again with the error message:
> >
> > WW(1) = 0. Stopping
> >
> > So maybe the shower analysis must be updated so as to work with your
> reweighting (check with the improved_scale_pdf_variation branch, maybe this is
> already done there).
> > In any case, please make sure that you can run a default shower analysis on
> top of your reweighting.
> >
> > d) In the card, I think it would be better to use another example than:
> >
> > #  SPECIFY A PATH OR USE THE SET COMMAND LIKE THIS:
> > 25 # set sminputs 1 130 # modify 1/alpha_EW
> >
> > because with the above, you will change the W mass with the default EW input
> scheme and this is typically a dangerous thing to do.
> >
> > e) I tried to reweight p p > e+ ve [QCD] in less conventional ways, with:
> >
> > change mode NLO    # Define type of Reweighting. For LO sample this command
> >                     # has no effect since only "LO" mode is allowed.
> > change model loop_qcd_qed_sm
> > change process u d~ > e+ ve QCD=99 QED=99 [virt=QCD QED]
> > change process c s~ > e+ ve QCD=99 QED=99 [virt=QCD QED] --add
> > launch
> >
> > But it crashed with:
> >
> > DEBUG: Command "reweight run_03 -from_cards" interrupted with error:
> > DEBUG: InvalidCmd : No particle qcd=99 in model
> >
> > I think it is important that any valid process syntax gets correctly parsed
> by the reweighting module.
> > Then I tried with
> >
> > change model loop_qcd_qed_sm
> > change process u d~ > e+ ve
> > change process c s~ > e+ ve --add
> > launch
> >
> > DEBUG: Command "reweight run_03 -from_cards" interrupted with error:
> > DEBUG: KeyError : ((2, 21), (-11, 1, 12))
> >
> > I guess I am missing some processes, but it would be nice if the error was
> more explicit!
> > So I tried again with something like :
> >
> > change model loop_qcd_qed_sm
> > change process p p > e+ ve
> > change process p p > e+ ve j --add
> > launch
> >
> > But this time crashed with:
> >
> > CRITICAL:  w_orig, w_new, w_new/w_orig =  4.3346035354e-05 4.33460282471e-05
> 0.999999836044 [reweight_interface.py at line 813]
> > DEBUG: Command "reweight run_03 -from_cards" interrupted with error:
> > DEBUG: KeyError : ''
> >
> > with backtrace
> >
> >  File "/Users/valentin/Documents/Work/MG5/nlo_reweighting/madgraph/interface
> /reweight_interface.py", line 611, in do_launch
> >    cross[name] += weight[name]
> > KeyError: ''
> >
> > Finally, I tried the 'change rwgt_dir' with:
> >
> > launch
> > set sminputs 1 100
> > change rwgt_dir
> /Users/valentin/Documents/Work/MG5/nlo_reweighting/PROCNLO_loop_sm_24/TMPKK
> > launch
> >
> > but it also crashed at the very end like this:
> >
> > DEBUG: Command "reweight run_03 -from_cards" interrupted with error:
> > DEBUG: IOError : [Errno 2] No such file or directory: '/Users/valentin/Docum
> ents/Work/MG5/nlo_reweighting/PROCNLO_loop_sm_24/TMPKK/rw_me/Cards/param_card.
> dat'
> > DEBUG: Please report this bug on https://bugs.launchpad.net/madgraph5
> > DEBUG: More information is found in '/Users/valentin/Documents/Work/MG5/nlo_
> reweighting/PROCNLO_loop_sm_24/run_03_reweight_2_debug.log'.
> > DEBUG: Please attach this file to your report.
> > REWEIGHT: Original cross-section: 10815.014 +- 40.594668 pb (cross-section
> from sum of weights: 10753.2535428)
> > REWEIGHT: Computed cross-section:
> > REWEIGHT: 2_nlo : 9398.36342752 +- 1734.09962991 pb
> > ERROR: I/O error (2): No such file or directory
> >
> > Ok, I will stop here with the non test of non-trivial reweighting, but I
> think that you should setup some acceptance/parallel tests which run with
> several different reweight cards covering all the options you claim to support
> on the wiki page and therefore make sure they pass.
> > Your reweighting is very powerful and advanced, and it uses *a lot* of
> different aspects of the MG5aMC codes, so that it is also very fragile. These
> all-inclusive tests are therefore necessary so that we will aware when some
> modification of the code breaks some aspect of your reweighting.
> >
> > f) The following syntax instruction in the reweight card is wrong:
> >
> > change process DEF [-add]:
> >
> > it should be:
> >
> > change process DEF [--add]:
> >
> > instead.
> >
> > g) Can I ask you a favor? Could you add your fortran wrapper that includes
> all relevant MadLoop functions as well as the f2py meta-codes in the
> standalone MadLoop output as well? (and make sure there is a target in the
> makefile to f2py it). I find it very cool and it would be nice to have it
> available in the standalone output (doesn't cost anything).
> >
> > h) Related to the point above, the reason why the intialization failed is
> because you have substituted check_sa.f !!!! So of course now it has not
> 'main' program statement and when you try to compile it, you get :
> >
> > cd ~/rw_mevirt/SubProcesses/P1_csx_epve
> > make check
> > ->
> > Undefined symbols for architecture x86_64:
> >  "_main", referenced from:
> >     implicit entry/start for main executable
> > ld: symbol(s) not found for architecture x86_64
> >
> > I think that the best behavior by far is to keep the original check_sa.f but
> add another fortran source file, like 'f2py_wrapper.f' (also the name is then
> much more explicit) and use this one in the makefile to build your shared
> object python module. That would be really nice if you could do this, and then
> also add 'f2py_wrapper.f' in the standalone output ;).
> >
> > Ok I stop there for now.
> > --
> > https://code.launchpad.net/~maddevelopers/mg5amcnlo/nlo_reweighting/+merge/2
> 83769
> > Your team MadDevelopers is subscribed to branch
> lp:~maddevelopers/mg5amcnlo/2.3.4.