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

avoid that mirroring is reset by the plugin #754

Merged
merged 1 commit into from
Aug 30, 2023
Merged

Conversation

oliviermattelaer
Copy link
Member

Hi @valassi,

This is a PR to fix the issue that
p p > t t~ returns the wrong cross-section (compare to pure madevent case: (see #678)

The issue that I found is that the plugin reset the "mirror" flag to False and since this is done "before" the creation of the madevent metadata file, the madevent loose track of such symmetry (and therefore of a part of the cross-section).
This explains why g g > t t~ was not impacted but that q q > t t~ was wrong by a factor of two.

The question is why did you implement such line (and what is going to break by removing such line).
To avoid too much side effect, I have just removed them for the madevent+cpp/cuda mode?
But we likely need to take a more drastic option here (but want your opinion on it)

Olivier

@oliviermattelaer
Copy link
Member Author

let's merge this and then see if we break something (release first fix later strategy)

@oliviermattelaer oliviermattelaer merged commit 6bf4a65 into master Aug 30, 2023
@oliviermattelaer oliviermattelaer deleted the fix_mirror branch August 30, 2023 12:57
@oliviermattelaer
Copy link
Member Author

This patch fixes the issue for the fortran mode but not for the cuda/cpp mode.
In the cpp mode, all the events written on disk have (IMIRROR=1)
and the reason for that is clear when running the ../madevent_cpp < input_app.txt

 selecting           1           1           1
 selecting           1           1           1
 selecting           1           1           1
 selecting           1           1           1
 selecting           1           1           2
WARNING! flagging abnormal ME for ievt=0
WARNING! flagging abnormal ME for ievt=1
WARNING! flagging abnormal ME for ievt=2
WARNING! flagging abnormal ME for ievt=3
WARNING! flagging abnormal ME for ievt=4
WARNING! flagging abnormal ME for ievt=5
WARNING! flagging abnormal ME for ievt=6
WARNING! flagging abnormal ME for ievt=7
WARNING! flagging abnormal ME for ievt=8
WARNING! flagging abnormal ME for ievt=9
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 Error in matrix element
 selecting           1           1           2
WARNING! flagging abnormal ME for ievt=0

So all event with imirror=2 have a cpp issue and returns 0 (and therefore cross-section is ~.5 the correct value and events are biased in the way they are written.

@oliviermattelaer
Copy link
Member Author

oliviermattelaer commented Aug 31, 2023

I guess that the issue is that the generated code for the amplitude is

      // Wavefunction(s) for diagram number 1
#if not( defined __CUDACC__ and defined MGONGPU_TEST_DIVERGENCE )
      imzxxx<M_ACCESS, W_ACCESS>( momenta, cHel[ihel][0], +1, w_fp[0], 0 ); // NB: imzxxx o\
nly uses pz
#else
      if( ( blockDim.x * blockIdx.x + threadIdx.x ) % 2 == 0 )
        ipzxxx<M_ACCESS, W_ACCESS>( momenta, cHel[ihel][0], +1, w_fp[0], 0 ); // NB: imzxxx\
 only uses pz
      else
        ixxxxx<M_ACCESS, W_ACCESS>( momenta, 0, cHel[ihel][0], +1, w_fp[0], 0 );
#endif

      omzxxx<M_ACCESS, W_ACCESS>( momenta, cHel[ihel][1], -1, w_fp[1], 1 ); // NB: opzxxx o\
nly uses pz

which due to the "mz" and "pz" assumes the sign of the z direction of the events.
This is problematic when using mirroring since those assumptions are wrong.

  • weirdly using ixxxxxx fixes the issue.

@valassi
Copy link
Member

valassi commented Oct 26, 2023

Hi @oliviermattelaer thanks for asking the review :-)
I had a look, in my tests I do not see the benefits (but I vaguely understand you may see a factor 2x in the xsec in your own tests), but I also see no problems. Or better, there were some hickups, but you fixed them in a later commit, so that's sll good for me.

valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 26, 2023
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
…nly changes are in codegen logs

Codebase includes merging commit a6731bd (Olivier Wed Aug 23 13:23:12 2023 +0200)
This uses Olivier's 'fix_mirror' branch for PR madgraph5#754
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
…matting, and especially the build will fail.

Codebase includes merging commit a6731bd (Olivier Wed Aug 23 13:23:12 2023 +0200)
This uses Olivier's 'fix_mirror' branch for PR madgraph5#754

In particular
  a6731bd       Olivier Mattelaer       Wed Aug 23 13:23:12 2023 +0200  Merge branch 'fix_mirror'
  2556cdd       Olivier Mattelaer       Wed Aug 23 09:27:38 2023 +0200  avoid that mirroring is reset by the plugin

These lines fail the build (as well as clang formatting)
[NOT OK] Check formatting in: pp_tt012j.mad/SubProcesses/P0_uux_ttx/CPPProcess.cc
786c786
<     constexpr int helcolDenominators[1] = { 36,36 }; // assume nprocesses == 1 (madgraph5#272 and madgraph5#343)
---
>     constexpr int helcolDenominators[1] = { 36, 36 }; // assume nprocesses == 1 (madgraph5#272 and madgraph5#343)
The same happens in each P subdirectory.

Build errors:
  ccache /usr/local/cuda-12.0/bin/nvcc   -Xcompiler -O3 -lineinfo -I. -I../../src -I/usr/local/cuda-12.0/include/ -DUSE_NVTX -gencode arch=compute_70,code=compute_70 -gencode arch=compute_70,code=sm_70 -use_fast_math -std=c++17  -ccbin /usr/lib64/ccache/g++ -DMGONGPU_FPTYPE_DOUBLE -DMGONGPU_FPTYPE2_DOUBLE -Xcompiler -fPIC -c gCPPProcess.cu -o gCPPProcess.o
  gCPPProcess.cu(779): error: static assertion failed with "Assume nprocesses == 1"
  gCPPProcess.cu(786): error: too many initializer values
  2 errors detected in the compilation of "gCPPProcess.cu".
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
…, previous issues are now solved

Codebase includes cherry-picking this commit (providing a fix for PR madgraph5#754)
  5b22a92       Olivier Mattelaer       Thu Aug 31 15:11:54 2023 +0200  fixing issue of stefan with nprocesses>1

Code changes are
  epochX/cudacpp/pp_tt012j.mad/SubProcesses/P0_gg_ttx/CPPProcess.cc
  <       static_assert( nprocesses == 1, "Assume nprocesses == 1" );
  <       // process_id corresponds to the index of DSIG1 Fortran functions (must be 1 because cudacpp is unable to handle DSIG2)
  ---
  >       static_assert( nprocesses == 1 || nprocesses == 2, "Assume nprocesses == 1 or 2" );
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
…ges (except for static assert allowing nprocesses==2)

This completes the checks for PR madgraph5#754, all looks good
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
…ter merging Stefan's fix for coupling order in PR madgraph5#757 - code changes only in gq_ttq

The new codebase includes in particular also
  6bf4a65       oliviermattelaer        Wed Aug 30 14:57:19 2023 +0200  Merge pull request madgraph5#754 from madgraph5/fix_mirror
  0aba0c6       oliviermattelaer        Wed Aug 30 14:53:17 2023 +0200  Merge pull request madgraph5#757 from roiser/fixcouplingordering
  4f3cdd8       Stefan Roiser   Tue Aug 29 17:43:25 2023 +0200  in PLUGIN_UFOModelConverter overwrite teh prepare_couplings function the additional functionality will re-order the dictionary keys such that they follow the order of the couplings in parameter wanted_couplings wanted_couplings contains the correct ordering as it is discovered in the OneProcessExporter class

NB: generated code is identical in all processes except for gq_ttq

NB: ee_mumu.mad code generation (and similary ee_mumu.sa) fails with
  Command "import /data/avalassi/GPU2023/madgraph4gpuX/MG5aMC/TMPOUT/CODEGEN_mad_ee_mumu.mg" interrupted in sub-command:
  "output madevent ../TMPOUT/CODEGEN_mad_ee_mumu --hel_recycling=False --vector_size=16384 --me_exporter=standalone_cudacpp" with error:
  KeyError : 'GC_3'
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Oct 28, 2023
This COMPLETES the merging of the current usptream.master into oct23av
It adds the following commits by Stefan and myself (PRs madgraph5#778 madgraph5#779)
6771781       Andrea Valassi  Thu Oct 26 20:16:42 2023 +0200  Merge pull request madgraph5#779 from valassi/ghav_minorfixes
59922dd       Andrea Valassi  Thu Oct 26 17:58:55 2023 +0200  [oct23av] in CODEGEN MatrixElementKernels.cc, fix clang-format after cherry-picking Olivier's 9fc9873
040f574       Andrea Valassi  Thu Oct 26 13:06:20 2023 +0200  [oct23av] in CODEGEN, improve comments to generated code for mirror processes (see PR madgraph5#754)
7564f04       Andrea Valassi  Thu Oct 26 20:06:29 2023 +0200  Merge pull request madgraph5#778 from valassi/ghav_gqttqref
d9c3fae       Andrea Valassi  Thu Oct 26 17:19:24 2023 +0200  [oct23av] in CODEGEN, update gqttq ref file for runTest after fixing coupling order in PR madgraph5#757
c803581       Stefan Roiser   Thu Oct 26 17:46:11 2023 +0200  sync submodule
@valassi valassi mentioned this pull request Jul 24, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 24, 2024
…(undo PR madgraph5#754) to fix madgraph5#872

This resets MIRROPROCS to FALSE (instead of TRUE) in mirrorprocs.inc files, e.g. in pp_ttjj
This fixes the fortran/cudacpp xsec mismatch madgraph5#872 in pp_ttjj
Note in particular that the numbers of events processed are now the same in Fortran and C++,
while a higher number of events was processed in Fortran without this fix

Revert "avoid that mirroring is reset by the plugin"
This reverts commit 2556cdd (from PR madgraph5#754)
Fix conflicts: epochX/cudacpp/CODEGEN/PLUGIN/CUDACPP_SA_OUTPUT/model_handling.py
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 24, 2024
…py to disable mirror processing again (undo PR madgraph5#754) to fix madgraph5#872

This does set MIRRORPROCS to false.
But it ALSO sets nprocesses=1 instead of nprocesses=1.
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 24, 2024
…ting nprocesses == 1 (part of the fix for madgraph5#872 in PR madgraph5#935; undo changes from PR madgraph5#754 and madgraph5#764)

NB: nprocesses is unused in cudacpp, because cudacpp is unable to handle mirror processes!
This means that the result of madevent with cudacpp MEs are the same whether nprocesses is 1 or 2.
However, nprocesses=2 signals that Fortran is using mirror processes.
If mirror processes are used in Fortran MEs but not in cudacpp MEs, this leads to a xsec mismatch madgraph5#872.

Implementing mirror processes in cudacpp would be extremely complicated.
This is an assumption that was made years ago.
To be cross checked with Olivier, but I think that the code generated with cudacpp without mirror processes
also ensure that the mirror processes are _separately_ generated as separate subprocesses also in Fortran,
therefore the whole calculation is internally consistent if mirrorprocesses is false in both Fortran and cudacpp.
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 25, 2024
…ck to upstream/master (i.e. do not undo PR madgraph5#754, this is needed, keep also madgraph5#764)

** NB bug madgraph5#872 xsec mismatch will show up again in the CI and in manual tests **

Revert "[pptt] in CODEGEN for CPPProcess.cc, add back the static_assert expecting nprocesses == 1 (part of the fix for madgraph5#872 in PR madgraph5#935; undo changes from PR madgraph5#754 and madgraph5#764)"
This reverts commit b5576ad.

Revert "[pptt] in CODEGEN model_handling.py, disable mirror processing again (undo PR madgraph5#754) to fix madgraph5#872"
This reverts commit b28e04b.
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.

2 participants