madgraph5 / madgraph4gpu

GPU development for the Madgraph5_aMC@NLO event generator software package
30 stars 33 forks source link

Add fbridgesequence_nomultichannel #796

Closed valassi closed 7 months ago

valassi commented 9 months ago

Hi @roiser @oliviermattelaer as discussed earlier on.

This is a MR that adds fortran call fbridgesequence_nomultichannel. It should be useful in Stefan's MR for trasnsforming channelid into an array.

As noted by @roiser, the issue is that fbridgesequence is presently called also with channelid=0 from the fortran (the integer is 0, not the pointer to the integer). https://github.com/madgraph5/madgraph4gpu/blob/b9f16e993eeac17f1eaaad3c1bc2a66c81e5e974/epochX/cudacpp/gg_tt.mad/SubProcesses/P1_gg_ttx/auto_dsig1.f#L565

        IF ( .NOT. MULTI_CHANNEL ) THEN
          CALL FBRIDGESEQUENCE(FBRIDGE_PBRIDGE, P_MULTI, ALL_G,
     &      HEL_RAND, COL_RAND, 0, OUT2,
     &      SELECTED_HEL2, SELECTED_COL2 ) ! 0: multi channel disabled

Technically it is possible from fortran to use a pointer (this is what we do for FBRIDGE_PBRIDGE via CppObjectInFortran struct), but it seems cumbersome here.

As noted by Stefan, better use a different function for the bridge sequence, this is what I have done here.

Note, initially I had misunderstood that the issue was eleiminating the #ifdef branch where multichannel is not supported. I tried this in the initial commits of this PR, but eventually this failed because it would break pure standalone code with no madevent, as there is no vcoloramps.h in that case and the code does not build. So I have also kept that anyway. Just to clarify, there are three use cases and they are all still supported

  1. multichannel code, with channelid>0 (presently a scalar, eventuallay an array via a pointer)
  2. multichannel code, with channelid=0 (presently a scalar, eventually a pointer=0 in cudacpp and a different function in fortran, i.e. this PR)
  3. no-multichannel code (no coloramps.h), is pure standalone generation

Stefan I ask you as reviewer. Up to you if you want to include this now before your PR, or cherry-pick inside your PR, or do your PR first and then we merge this on top (I can fix teh conflicts in tht case).

Thanks Andrea

valassi commented 9 months ago

For an easier review, just look at this https://github.com/madgraph5/madgraph4gpu/pull/796/commits/d5c4c1b1da094fb226ea2b5555ff45c4b73d986a

The rest is code generation changes, and process regeneration

valassi commented 8 months ago

I have just merged https://github.com/madgraph5/madgraph4gpu/pull/706, I will modify this PR to merge in the new master

valassi commented 7 months ago

Hi @oliviermattelaer this PR is also complete. It is one small ingredient of the work you are doing with @roiser to have many channels on the same grid. I would suggest putting this in independently of Stefan's work and doing it now. Otherwise every time there is a need to update to the latest master, retest etc.

Can you please @oliviermattelaer review and approve for merge?

As mentioned above the main change is just this one https://github.com/madgraph5/madgraph4gpu/commit/d5c4c1b1da094fb226ea2b5555ff45c4b73d986a which is really simple

Thanks Andrea

roiser commented 7 months ago

Actually with the latest changes that we need to make (no channel ids for CPU) I would propose to postpone this PR a bit if you don't mind. I haven't thought it through yet what the implications are.

valassi commented 7 months ago

Thanks @roiser. No I would very much prefer to merge this now. Having all these PRs in flight forces me to spend a large effort fixing conflicts and retesting. The sooner we get rid of open PRs so that we only have a more manageable number, the better.

Concerning this specific PR: it is so simple that it should not be a problem in the work you are doing now (and actually you asked for this initially so it cannot be bad!). It just adds a new function for the no-multichannel case and uses it where appropriate, in helicity filtering.

I would really merge this now. @oliviermattelaer what's your take?

Thanks Andrea

valassi commented 7 months ago

PS The fact that this now appears as not-mergeable is because I just merged PR #368. Probably easy conflicts to fix, but again I would rather not have to do this all the time. The sooner we merge this the better.

roiser commented 7 months ago

I'm just asking for postponing it by one or two days, the latest incompatibilities only came up on Friday, I can also incorporate it into the bigger channelID work if wanted.

valassi commented 7 months ago

I'm just asking for postponing it by one or two days, the latest incompatibilities only came up on Friday,

Ok for one or two days then...

I can also incorporate it into the bigger channelID work if wanted.

I would still find it cleaner to have separate PRs for separate issues - in general. If you want I can reduce this to essentially one cherry-pick in the CODEGEN part, and hide away all changes in generated code and test logs, so that it's easier to merge. It becomes equivalent to a cherry-pick on your side.

roiser commented 7 months ago

I'm just asking for postponing it by one or two days, the latest incompatibilities only came up on Friday,

Ok for one or two days then...

I looked through the code and found one possibly minor issue, please have a look at the comment above. Otherwise I imagine this should work. @oliviermattelaer you would need to pass me an INTEGER*4 (*) in cases the channel id is used. Following our latest discussion IIUC for CPU this would be only of length 1, is this possible for you to distinguish?

roiser commented 7 months ago

please go ahead then, thanks Stefan

valassi commented 7 months ago

please go ahead then, thanks Stefan

Thanks I will merge then