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="RFG-NucRemovalE@Pdg=1000280580"> 0.0360 </param> <!-- Ni58 --> | ||
| <param type="double" name="RFG-NucRemovalE@Pdg=1000822080"> 0.0440 </param> <!-- Pb208 --> | ||
| <param type="double" name="RFG-NucRemovalE@Pdg=1000501190"> 0.0280 </param> <!-- Sn119 --> | ||
| <param type="double" name="RFG-NucRemovalE@Pdg=1000791970"> 0.0310 </param> <!-- Au197 --> |
There was a problem hiding this comment.
Can the addition of Sn119 and Au197 be put in order of increasing Z, that is before Pb208.
|
|
||
| <!-- <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?
| * d+//+d my+/smmyhN m///h NEUTRINO MONTE CARLO GENERATOR * | ||
| * Ns///yN NdyoshNNs///d h////yN * | ||
| * mo//om ms///+m d///////oyhmN Version 999.999.999 * | ||
| * mo//om ms///+m d///////oyhmN Version 3.06.02 * |
There was a problem hiding this comment.
Please leave this as 999.999.999 which signifies that this is an untagged setup on the main branch. This version number will be updated and explicitly set when it comes to tag. These changes won't be in the 3.06.02 tag because that tag is already set in stone.
| ** d8P' `Y8b `888' `8 `888b. `8' `888' `888' `8 NEUTRINO MONTE CARLO GENERATOR ** | ||
| ** 888 888 8 `88b. 8 888 888 ** | ||
| ** 888 888oooo8 8 `88b. 8 888 888oooo8 Version 999.999.999 ** | ||
| ** 888 888oooo8 8 `88b. 8 888 888oooo8 Version 3.06.02 ** |
There was a problem hiding this comment.
Again, leave the version as 999.999.999. This will get updated when there is an actual tag with these (and any other) changes.
| @@ -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 |
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.