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.

> 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….

> 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 guess I am missing some processes, but it would be nice if the error was more explicit!

That is fine like that 

> 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.


> 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.

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/Documents/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/283769
> Your team MadDevelopers is subscribed to branch lp:~maddevelopers/mg5amcnlo/2.3.4.