Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix RESKinematicsGenerator #291

Merged
merged 9 commits into from
Jul 25, 2023

Conversation

idkakorin
Copy link
Contributor

Fixing of the problem with RESKinematicsGenerator reported in issue #282 .

I suggest getting rid of the variable QD2 to avoid confusion with Jacobian, variable transformation and global maximum searching. The function double genie::utils::kinematics::RESImportanceSamplingEnvelope(double * x, double * par) is used only in RESKinematicsGenerator.cxx, therefore it should not cause problems anywhere else. All changes are trivial.

Copy link
Member

@mroda88 mroda88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me, have we done some testing and validation of the new generation?
Can we post it here?

@candreop
Copy link
Member

Hi Igor: the transformation from Q2 to Q2_d = 1/(1+Q2/c), was done to flatten the differential cross-section by taking out the dipole form. It is easy to remove the transformation and the code would work. But what is the impact on efficiency? If it now becomes much slower, perhaps we want to keep it and fix what is wrong with the sampling envelope? But we need some assessment of CPU efficiency first. Please can you check how fast you can generate RES events w/ and w/o that transformation?

@idkakorin
Copy link
Contributor Author

idkakorin commented Jun 14, 2023

Hi Igor: the transformation from Q2 to Q2_d = 1/(1+Q2/c), was done to flatten the differential cross-section by taking out the dipole form. It is easy to remove the transformation and the code would work. But what is the impact on efficiency? If it now becomes much slower, perhaps we want to keep it and fix what is wrong with the sampling envelope? But we need some assessment of CPU efficiency first. Please can you check how fast you can generate RES events w/ and w/o that transformation?

Hi Costas! I understand your point about flattering, but straightforward comparison of previous version and proposed fix would not be quite correct, because in the previous version of code some of the events are accepted, although they should have been rejected. Anyway, such comparison shows, that without fix time/event: 0.00597427 s, with fix: 0.0499688 s.

@idkakorin
Copy link
Contributor Author

Now, the maximum is searched in W, QD2 variables. We decided to get rid of envelope.
The test runs of the command:
time gevgen -p 14 -t 1000010010 -n 10000 -e 0.1,100 -f 1 --cross-sections out.xml --tune G00_00a_00_000 --event-generator-list RES
showed that it works faster with this option, see Presentation.pdf.

@mroda88 mroda88 requested a review from dytman July 7, 2023 15:10
Copy link
Member

@mroda88 mroda88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by Costas in the Core developer meeting of the 7 July 2023

@mroda88 mroda88 merged commit 971a8b7 into GENIE-MC:master Jul 25, 2023
@mroda88 mroda88 deleted the fix_RESKinematicsGenerator branch July 25, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants