larsb-p GENIE(Reweight) development#478
Conversation
…d added modified G18_10a tunes that only differ in their 2p-2h models (a: Valencia, e: Empirical, s: SuSAv2, m: Martini)
…ir CommonParam.xml files
nusense
left a comment
nusense
left a comment
There was a problem hiding this comment.
Okay, I've made some comments of things that need to be updated/fixed. But I abandoned this review after 54 of 66 files because I realized that it affects many (but not all) of the same files modified by pull request #472 by @lavinia-russo which I eventually "approved" three weeks ago. Since I don't know either @lavinia-russo nor @Lars-P I don't know what's going on here, but the two of you need to coordinate and come to some resolution of what is to be changed in a unified manner.
|
|
||
| <!-- <param type="double" name="DIS-CC-XSecScale"> 1.032 </param> --> | ||
| <!-- <param type="double" name="MEC-CC-XSecScale"> 1.000 </param> --> | ||
| <!-- <param type="double" name="MEC-CC-XSecScale"> 1.000 </param> --> |
There was a problem hiding this comment.
An erroneous removal of a space here? In GitHub the comments no longer line up in their indentation.
| <param type="int" name = "gsl-max-eval"> 100000 </param> | ||
| <param type="alg" name = "VertexGenAlg"> genie::VertexGenerator/Default </param> | ||
| <param type="int" name = "NumNucleonThrows"> 500 </param> | ||
| <param type="int" name = "NumNucleonThrows"> 5000 </param> |
There was a problem hiding this comment.
Generally it is better to have more throws, but it's always a trade-off of cpu vs accuracy. Do you know the impact of increasing this from 500 to 5000, in terms of performance?
| @@ -0,0 +1,42 @@ | |||
| //____________________________________________________________________________ | |||
| /* | |||
| Copyright (c) 2003-2023, The GENIE Collaboration | |||
There was a problem hiding this comment.
This code is going to be released in 2026. You should update the copyright date in all your files to reflect this.
| Copyright (c) 2003-2023, The GENIE Collaboration | ||
| For the full text of the license visit http://copyright.genie-mc.org | ||
| or see $GENIE/LICENSE | ||
| Author: Steven Gardiner <gardiner \at fnal.gov> |
There was a problem hiding this comment.
I'm not sure who lars-p is, but I'm pretty sure it's not Steven Gardiner. Is this author attribution correct or a carry over from boiler plate used to start this file? Again, verify it for all the new files.
| cross sections using the Martini approach | ||
| \author Steven Gardiner <gardiner \at fnal.gov> | ||
| Fermi National Accelerator Laboratory | ||
| \created April 26, 2019 |
There was a problem hiding this comment.
Is this created date also right, or a carry over?
| this -> AddTargetRemnant(event); | ||
| event->Print(); | ||
| this -> GenerateNSVInitialHadrons(event); | ||
| event->Print(); |
There was a problem hiding this comment.
I really don't expect that we want "production" code calling event->Print() by default four time for every time we enter this block. I presume this is debugging code and should be cleaned up.
| // cross section blows up as Q^2 --> 0) | ||
| double Q2min = genie::controls::kMinQ2Limit; // CC/NC limit | ||
| if ( interaction->ProcInfo().IsEM() ) Q2min = genie::utils::kinematics | ||
| ::electromagnetic::kMinQ2Limit; // EM limit |
There was a problem hiding this comment.
I already bugged someone in another pr about this very ugly line break which seems to get repeated. Could you have this instead be:
if ( interaction->ProcInfo().IsEM() )
Q2min = genie::utils::kinematics::electromagnetic::kMinQ2Limit; // EM limit
Breaking the fully qualified name in the middle is difficult to read/interpret
|
The title of this PR also says |
larsb-p
changed the title
…au to relax the requirement that all RFG-NucRemovalE parameters have to present in the CommonParam.xml file by changing GetParam function to GetParamDef function in the respective lines of both files. Then, removed these lines in the CommonParam.xml files of the GENIE tunes to revert to original CommonParam.xml files with He, Sn and Au.
…ng hard-coded xsec ratio tables to it reading the xsecs directly from the user's input cross-section spline xml-file. The user can now input multiple xml-files via the cross-sections option of grwght1p (modified in src/Framework/Utils/XSecSplineList.h). Also a bug was removed in the CommonParam.xml files.
nusense
left a comment
nusense
left a comment
There was a problem hiding this comment.
Added one more comment. Resolved a couple of previous comments. But there are other previous comments that must be addressed before we can incorporate this into the code base: cf. the hardcoding a old version number instead of leaving it at 999.999.999, and calls to event->Print() in MECGenerator.cxx. It would be nice to have the other comments also addressed or at least replied to.
| } | ||
|
|
||
| if( ! good_config ) { | ||
| LOG("MartiniEricsonChanfrayMarteauMECPXSec2024", pERROR) << "Configuration has failed."; |
There was a problem hiding this comment.
If you're going to call exit() the preceding message should have priority pFATAL
…99 and removed event->Print() in Physics/Multinucleon/EventGen/MECGenerator.cxx)
nusense
left a comment
nusense
left a comment
There was a problem hiding this comment.
There are a couple comments that are more minor that aren't blockers, but all the critical comments have been addressed. I'l approve this at this point
3 participants
My GENIE branch "larsb-p_Martini_GENIEReweight_version" is necessary to use the 2p-2h systematic uncertainty parameter implementation in GENIEReweight implemented in branch "larsbp_update_to_GENIEv3_06_02". The main changes include the addition of He4, Sn119 and Au197 into config/CommonParam.xml as well as the Martini-Ericson-Chanfray-Marteau implementation.