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

oliviermattelaer commented 3 months ago

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 commented 3 months ago

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 https://github.com/madgraph5/madgraph4gpu/blob/befbfdfad61595a5f5b612635770349b55f01f41/.github/workflows/testsuite_oneprocess.sh#L534

# 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 commented 3 months ago

Thanks,

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

oliviermattelaer commented 3 months ago

@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 commented 3 months ago

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 https://github.com/mg5amcnlo/mg5amcnlo/compare/1e2aa4bc3b19b53de2249b82b06f834299c4a23e...493fe456ff75743422e8d33c9d9b6dabbb3c8608#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 commented 3 months ago

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.

valassi commented 3 months ago

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

oliviermattelaer commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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)

oliviermattelaer commented 3 months ago

Yes, it can be merged. Thanks a lot,

Olivier

valassi commented 3 months ago

Very good, thanks to you. Merging.

madgraph5 / madgraph4gpu

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