Skip to content

ensure that cpp data structure for is_LC is the same in fortran and cpp#880

Merged
valassi merged 6 commits intomasterfrom
datastructure_coloramp
Jul 4, 2024
Merged

ensure that cpp data structure for is_LC is the same in fortran and cpp#880
valassi merged 6 commits intomasterfrom
datastructure_coloramp

Conversation

@oliviermattelaer
Copy link
Member

Adding a PR (in WIP) to see if the following branch does create issue.
@valassi do we have a CI test for the missmatch of color?
Because, the point of this PR is to be able to fix such missmatch (I did not tested that part yet).

In particular, I want to test if I did break things for SA side (It would be weird...)

The idea of such patch is to ensure that the datastructure use to write the color (is_LC information) is the same on the fortran and cpp side.

In the previous code, the function was called twice with two different type of data-structure. I do not understand why this was needed, so I fully removed the first call (which explains why I want to run the CI to check for side effect). (and change the datastructure of the second call)

@valassi
Copy link
Member

valassi commented Jul 3, 2024

Adding a PR (in WIP) to see if the following branch does create issue.
@valassi do we have a CI test for the missmatch of color?
Because, the point of this PR is to be able to fix such missmatch (I did not tested that part yet).

Hi Olivier, thanks a lot.

Yes we have a CI, but I also have bypasses for 'known issues' to make sure that things are green otherwise (bit silly maybe... only needed till we fix everything).

In your case now I think the bypasses are active, you need to comment out these three lines an din particular the middle one for 856

if [ $BYPASS_KNOWN_ISSUES -eq 1 ] && [ $status -ne 0 ]; then

# No cross section in susy_gg_t1t1 (#826)
    if [ "${proc%.mad}" == "susy_gg_t1t1" ]; then bypassIssue "No cross section in ${proc%.mad} for FPTYPE=d,f,m (#826)"; fi
    # LHE color mismatch in gg_ttgg for iconfig=104 (#856)
    if [ "${proc%.mad}" == "gg_ttgg" ]; then bypassIssue "LHE color mismatch for iconfig=104 in ${proc%.mad} for FPTYPE=d,f,m (#856)"; fi
    # Cross section mismatch in pp_tt012j for P2_gu_ttxgu (#872)
    if [ "${proc%.mad}" == "pp_tt012j" ]; then bypassIssue "Cross section mismatch for P2_gu_ttxgu in ${proc%.mad} for FPTYPE=d,f,m (#856)"; fi

(Rephrasing: all tests are succeeding now, but it may be due to bypasses... though from the actual log I have the impression the test was actually succeesful. Comment out all three lines, you will have 6 tests failing if it worked, 9 failing if it did not work)

@oliviermattelaer
Copy link
Member Author

Thanks,

Let's hope the test will crash now (but only 6 times) :-)

@oliviermattelaer oliviermattelaer marked this pull request as ready for review July 4, 2024 04:47
@oliviermattelaer oliviermattelaer requested a review from a team as a code owner July 4, 2024 04:47
@oliviermattelaer oliviermattelaer requested a review from valassi July 4, 2024 04:48
@oliviermattelaer
Copy link
Member Author

@valassi This should be ready for review.
The change within the mg5amcnlo repo is here optional (the CI passes with and without such changes).
I would say that it is cleaner to include both fix (but no strong feeling).

It might also be better to encapsulate the new line of code that I put in this plugin in the mg5amcnlo code (in a new function) to avoid repetition if you agree with that, then I can do it right away.

Thanks,

Olivier

@valassi
Copy link
Member

valassi commented Jul 4, 2024

The change within the mg5amcnlo repo is here optional (the CI passes with and without such changes). I would say that it is cleaner to include both fix (but no strong feeling).

It might also be better to encapsulate the new line of code that I put in this plugin in the mg5amcnlo code (in a new function) to avoid repetition if you agree with that, then I can do it right away.

Thanks Olivier very good! I will test this (will take me a bit of time).

Concerning the MG5AMC change
mg5amcnlo/mg5amcnlo@1e2aa4b...493fe45#diff-16f27b34d4b29e5eee7c9b517bed11114a35f00aed406095d470691dd6f8e063L1521
This is the removal of this line

            multi_channel = self.get_multi_channel_dictionary(self.matrix_elements[0].get('diagrams'), self.include_multi_channel)
-           replace_dict['is_LC'] = self.get_icolamp_lines(multi_channel, self.matrix_elements[0], 1)
            replace_dict['nb_channel'] = len(multi_channel)

Your take on this, I have no strong feeling.

I think that the question is:

  • should export_cpp.py be able to work by itself, without cudacpp
  • or are we now moving to a situation where export_cpp ONLY makes sense in combination with cudacpp?

In the first case, you should probably NOT remove the line as otherwise export_cpp without cudacpp would not work. But I am not going to test that ;-)

In the second case, which is probably the truth, then I guess it is indifferent. Eventually we might want to remove export_cpp and merge it into cudacpp, or viceversa (or in any case a good cleanup would be useful). But here it's really up to you to suggest/choose a direction... let me know!

PS But ok, maybe all in all I have a slight prefernce for not removing the line. But again, your take... If we leave it in, the code will not crash in python, but will produce wrong results in generated code. Maybe what you were suggesting was to have a single implementation in export_cpp, ie move your current fix from cudacpp to export_cpp? That might make sense. BUt again your take...

@oliviermattelaer
Copy link
Member Author

To answer your question, I need to distinct multiple part of export_cpp

  • They are many part of export_cpp that should be able to work without the plugin (like the pythia8 output, the "old" standalone_cpp output, ...)
  • Then we have the gpu exporter that we used during the first hackathon and which is not working as a standalone for the moment. So yes, we do have opportunity for cleaning that part of the code. But this is not urgent/critical for the moment.

So concerning the code style, I take your comment as a go ahead to do a bit of code refactorization,
which can also help a bit more for the decision to keep or not those lines. So I will put this back as WIP, my goal here will be to not modified the generated code.

@oliviermattelaer oliviermattelaer marked this pull request as draft July 4, 2024 07:59
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
…that Olivier's PR madgraph5#880 fixes madgraph5#856

./tmad/teeMadX.sh -ggttgg -makeclean +10x
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
…for issue madgraph5#856 - now including fixes in icolamp in coloramps.h files
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
@oliviermattelaer oliviermattelaer force-pushed the datastructure_coloramp branch from d580c9a to 85be439 Compare July 4, 2024 09:32
Copy link
Member

@valassi valassi left a comment

Choose a reason for hiding this comment

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

Hi Olivier, this is good to go for me.

I tested it in #881, which also adds code regeneration (and which I would merge after we merge this one).

Befre merging I will wait for your latest MG5AMC changes.

Thanks again!

@valassi
Copy link
Member

valassi commented Jul 4, 2024

I have updated and closed #856 (I tried to make it clear that it was closed here in #880, not in #877 or mg5amcnlo/mg5amcnlo#116)

@oliviermattelaer
Copy link
Member Author

Thanks @valassi,
Here is the refactoring change.

However I have experimented with "rebase" ( I know that I always said not to do it to @roiser. don't tell him, he might not be listening) I hope it will not complicated things on your side (sorry if it does, I should have known better...)

Olivier

@valassi
Copy link
Member

valassi commented Jul 4, 2024

However I have experimented with "rebase" ( I know that I always said not to do it to @roiser. don't tell him, he might not be listening) I hope it will not complicated things on your side (sorry if it does, I should have known better...)

Ha ha dont worry :-)

I am retesting my stuff (with plentu of rebase and force push and other uglier stuff!).

Is this the final thing on your side? Can I merge this if my tests are ok? (Then I will later ask you to review #881)

@valassi
Copy link
Member

valassi commented Jul 4, 2024

Is this the final thing on your side? Can I merge this if my tests are ok?

Maybe remove the draft status and mark it ready for review when ready please, then I will just merge (I confirm that it looks ok)

valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
…that Olivier's PR madgraph5#880 fixes madgraph5#856

./tmad/teeMadX.sh -ggttgg -makeclean +10x
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
…for issue madgraph5#856 - now including fixes in icolamp in coloramps.h files
valassi added a commit to valassi/madgraph4gpu that referenced this pull request Jul 4, 2024
@oliviermattelaer oliviermattelaer marked this pull request as ready for review July 4, 2024 11:38
@oliviermattelaer
Copy link
Member Author

Yes, it can be merged. Thanks a lot,

Olivier

@valassi
Copy link
Member

valassi commented Jul 4, 2024

Very good, thanks to you. Merging.

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.

MAJOR ISSUE: color mismatch fortran/cpp in LHE file for iconfig 104 in SM gg_ttgg (channel/iconfig mapping AND icolamp issues)

2 participants