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

Further fixes/improvements for iconfig-channel mappings in coloramps.h #877

Merged
merged 101 commits into from
Jul 3, 2024

Conversation

valassi
Copy link
Member

@valassi valassi commented Jul 1, 2024

Hi @oliviermattelaer this is a modified version of #873, to be compared to your fix_826. It is built on top of your fix_826 PR #852, so it has by construction no conflicts with that. (So it can be compared to fix_826 as you suggested in #873 (comment))

As discussed offline I would suggest to agree upfront on this one and on mg5amcnlo/mg5amcnlo#116, and then:

Thanks!
Andrea

oliviermattelaer and others added 30 commits May 31, 2024 16:47
…p and gpucpp_826, to allow cherry-picking Olivier's fix_826 changes

(later on, will include Olivier's gpucpp_826 change into gpucpp directly)

Revert "[tmad] update mg5amcnlo to f274cab55, adding volatile to prevent madgraph5#855 crashes in rotxxx (move this upstream as suggested by Olivier)"
This reverts commit 720ae02.

Revert "[valgrind] upgrade MG5AMC to include the merge of PR madgraph5#110 and PR madgraph5#112 into the gpucpp branch"
This reverts commit 7d3dc34.

Revert "[valgrind] upgrade MG5AMC to include the workaround for uninitialised values mg5amcnlo/mg5amcnlo#111"
This reverts commit f355965.

Revert "[valgrind] upgrade MG5AMC to include the fix for memory leak mg5amcnlo/mg5amcnlo#109"
This reverts commit 7bb4142.
… now including Olivier's extra commit previously in gpucpp_826

This should complete the merge in madgraph4gpu master of the gpucpp_826 and gpucpp branches of MG5AMC.
It should now be possible to remove the gpucpp_826 branch from MG5AMC.
…y branch (WIP PR madgraph5#853) to allow cherry-picking the next commit

git checkout 335d6d2 susy_gg_t1t1.mad/CODEGEN_mad_susy_gg_t1t1_log.txt
…rt (just check that the merge so far was ok)
Revert "[color] temporarely regenerate susy_gg_t1t1.mad, no change, will revert (just check that the merge so far was ok)"
This reverts commit dbfb3bc.
…rangely, clang formatting was not complaining?)
… comma is allowed in the std::map initializer list
Note: cuda compilation 'make -j BACKEND=cuda' fails with
  coloramps.h(14): error: dynamic initialization is not supported for a __device__ variable
…p as constexpr, this fails with a different error

ccache /usr/local/cuda-12.0/bin/nvcc  -I. -I../../src  -Xcompiler -O3 -gencode arch=compute_70,code=compute_70 -gencode arch=compute_70,code=sm_70 -lineinfo -use_fast_math -I/usr/local/cuda-12.0/include/ -DUSE_NVTX  -std=c++17  -ccbin /usr/lib64/ccache/g++ -DMGONGPU_FPTYPE_DOUBLE -DMGONGPU_FPTYPE2_DOUBLE -Xcompiler -fPIC -c -x cu CPPProcess.cc -o CPPProcess_cuda.o
coloramps.h(14): error: expression must have a constant value
coloramps.h(14): note #2703-D: cannot call non-constexpr function "std::map<_Key, _Tp, _Compare, _Alloc>::map(std::initializer_list<std::map<_Key, _Tp, _Compare, _Alloc>::value_type>, const _Compare &, const std::map<_Key, _Tp, _Compare, _Alloc>::allocator_type &) [with _Key=int, _Tp=int, _Compare=std::less<int>, _Alloc=std::allocator<std::pair<const int, int>>]"
/usr/include/c++/11/bits/stl_map.h(228): here
coloramps.h(14): error: a constexpr variable must have a literal type or a reference type
CPPProcess.cc(1012): error: no operator "[]" matches these operands
            operand types are: const std::map<int, int, std::less<int>, std::allocator<std::pair<const int, int>>> [ const unsigned int ]
3 errors detected in the compilation of "CPPProcess.cc".
…making the map constexpr

Revert "[susy] in susy_gg_t1t1.mad, try to fix coloramps.h by defining the map as constexpr, this fails with a different error"
This reverts commit 38b3afd.
…ncludes clang format fixes)

This code now compiles... but it does not seem to fix the empty cross section problem
…graph5#826 is NOT fixed) after Olivier's first two commits in PR madgraph5#852

./tmad/teeMadX.sh -susyggt1t1 +10x -makeclean

(PS: it is NOW quite obvious to me that PR madgraph5#852 should be tested against different issues like ggttgg madgraph5#856, NOT against susy madgraph5#826)
(PS: will now therefore slowly move to different tests - first, however, recover my coloramps.h proposal in susy and Olivier's later changes)
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 3, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 3, 2024
…moving the icolamp fix from MG5aMC to cudacpp

NB: this is now functionally equivalent to the previous final version a245daf of color2
Will now remove the fix for madgraph5#856 as suggested by Olivier for PR madgraph5#877 - tests will fail
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 3, 2024
…5#856 as suggested by Olivier in PR madgraph5#877

Will not rerun all tput/tmad tests manually as many are expected to fail.
The CI will show many failing tests (variants of madgraph5#856).
@valassi
Copy link
Member Author

valassi commented Jul 3, 2024

Could you change the commit associated to this PR to a commit that does not contain it (gpucpp should be fine).

I have now done this, so that we can finally merge this and PR #852.

For the moment I moved back to mg5amcnlo/mg5amcnlo@09c96dd. I will later move to the latest gpucpp commit mg5amcnlo/mg5amcnlo@1e2aa4b as suggested by Olivier.

I have removed the association to #856 because THIS PR WILL NOT FIX #856. It contains a fix (moved to cudacpp), but then also its reverting.

image

The CI test shows 9 errors in https://github.com/madgraph5/madgraph4gpu/actions/runs/9776063937

(I also suspect more errors with LHE files similar to 856 would show up if running on different iconfig).

As suggested by Olivier I will include my suggested fix for 856 in my own master_valassi fork, which I will then need to create.

I will add back the bypass for 856, so that the CI checks for new issues only.

valassi added 10 commits July 3, 2024 13:47
…xcept for cosmtic formatting) from export_cpp

(Prepare to move into cudacpp the fix for madgraph5#856 previously in unmerged PR mg5amcnlo/mg5amcnlo#116)
…moving the icolamp fix from MG5aMC to cudacpp

NB: this is now functionally equivalent to the previous final version a245daf of color2
Will now remove the fix for madgraph5#856 as suggested by Olivier for PR madgraph5#877 - tests will fail
…#114 as suggested by Olivier - TMAD TESTS WILL FAIL

Revert "[color2] in CODEGEN model_handling.py, add the fix for madgraph5#856 and for mg5amcnlo/mg5amcnlo#114 which was previously in mg5amcnlo/mg5amcnlo#116"
This reverts commit 2daad8f.

Revert "[color2] in CODEGEN model_handling.py, add get_icolamp_lines as-is (except for cosmtic formatting) from export_cpp"
This reverts commit 705b5eb.
…5#856 as suggested by Olivier in PR madgraph5#877

Will not rerun all tput/tmad tests manually as many are expected to fail.
The CI will show many failing tests (variants of madgraph5#856).
…es for known issues

The CI tests are expected to fail for madgraph5#826 in susy and madgraph5#872 in pp_tt012j
In addition, madgraph5#856 and similar issues with LHE color mismatches are expected to appear
…aph5#856 (commented out) to known issues in the CI as its fix has been removed
…bc3 in the gpucpp branch (improving the comments around 'goodjet' valgrind warnings mg5amcnlo/mg5amcnlo#111)
…s/testsuite_oneprocess.sh, reenaable bypasses for known issues - the CI is now expected to pass
@valassi valassi marked this pull request as ready for review July 3, 2024 12:28
@valassi valassi changed the title Fixes for iconfig-channel mappings in coloramps.h Further fixes/improvements for iconfig-channel mappings in coloramps.h Jul 3, 2024
@valassi
Copy link
Member Author

valassi commented Jul 3, 2024

I will later move to the latest gpucpp commit mg5amcnlo/mg5amcnlo@1e2aa4b as suggested by Olivier.

I have now moved to the latest gpucpp commit as suggested by Olivier.
(Again, this does NOT include the fix in 116 instead: it does include the full fix for #856).

image

Let me approve this already and then check the associated PR for the other repo.

I have merged PR #852 "fix_826". Since Olivier approved this (an earlier version - I believe this has the further improvements he required) I will now merge this one as well, with further fixes/improvements.

So I guess that while mg5amcnlo/mg5amcnlo#116 is fine (up to some cleaning), it is not fine to refer to it within this PR. Could you change the commit associated to this PR to a commit that does not contain it (gpucpp should be fine).

Specifically, this is now done.

Thanks a lot again Olivier for all the patient discussion on this
Merging. Andrea

@valassi valassi merged commit 6ba6bae into madgraph5:master Jul 3, 2024
169 checks passed
@valassi
Copy link
Member Author

valassi commented Jul 3, 2024

(PS The CI for the version I finally merged is green because it contains bypasses for three known issues #826 #872 and #856)

valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 3, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
… that madgraph5#856 is not yet fixed in upstream/master, as expected after madgraph5#877

./tmad/teeMadX.sh -ggttgg -makeclean +10x
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
…er merging

Note: these are the tmad logs where madgraph5#856 was fixed in a previous attempt during madgraph5#877
git checkout 46356d6 tmad/logs_*
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
… that madgraph5#856 is not yet fixed in upstream/master, as expected after madgraph5#877

./tmad/teeMadX.sh -ggttgg -makeclean +10x
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
…er merging

Note: these are the tmad logs where madgraph5#856 was fixed in a previous attempt during madgraph5#877
git checkout 46356d6 tmad/logs_*
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