madgraph5 / madgraph4gpu

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

Remove htuple.f from generated code #967

Open valassi opened 1 month ago

valassi commented 1 month ago

Hi @oliviermattelaer I just realised that there are two functions ntuple with the same API, one in ranmar.f and one in htuple.f.

This is extremely confusing. In fact yesterday evening I thought that all of madgraph was using the htuple algorithm (a quasi random generator based on scrambled hanson) rather than ranmar. This was also rather scary because there is no seed change in htuple.f.

Now I think I understand that the ranmar version is used (and randinit is propagated correctly there).

So my suggestion is, can we remove htuple.f from generated code? I can do that, easy, but I'd need a confirmation please.

I did a quick test where I removed htuple.f and in fact the code builds fine.

Next question would be, should htuple.f be removed in upstream mg5amcnlo or in the plugin... I can do both

Thanks Andrea

oliviermattelaer commented 1 month ago

Hi,

This is the kind of stuff that I prefer not to touch. Since the probability to break something is high and the probability of conflict with other MG5 branch high as well (in particular madnis here).

So a "simple" check of compilation is not good enough (one need to test all the cases where madevent is used (i.e. all the possible way to build madevent code, like helicity recycling, no grouping, loop-induced, madweight) I just spend one month to fix all the stuff that we did broke in all those modes so not so motivated to restart such work (and I spotted another segfault yesterday night :-(, so not yet finished)

Concerning the plugin, my strategy is that we should maintain it in the long term. and therefore: 1) minimize patch 2) only change what we need to change (so the matrix-element and the bridge)

So I would avoid to do that in the plugin and indeed propose such change via a PR in MG5aMC. But such PR will need a quite deep and long scrutiny (i.e. I doubt that we can put that in/for 3.6.0 and therefore for the release of the plugin).

Maybe a better PR for the moment is to put a depreciation warning or similar to avoid breaking things and help to build confidence in the fact that this can be removed. We obviously need to be careful given how many person are using this tool.

Cheers,

Olivier

On 15 Aug 2024, at 11:32, Andrea Valassi @.***> wrote:

Hi @oliviermattelaerhttps://github.com/oliviermattelaer I just realised that there are two functions ntuple with the same API, one in ranmar.f and one in htuple.f.

This is extremely confusing. In fact yesterday evening I thought that all of madgraph was using the htuple algorithm (a quasi random generator based on scrambled hanson) rather than ranmar. This was also rather scary because there is no seed change in htuple.f.

Now I think I understand that the ranmar version is used (and randinit is propagated correctly there).

So my suggestion is, can we remove htuple.f from generated code? I can do that, easy, but I'd need a confirmation please.

I did a quick test where I removed htuple.f and in fact the code builds fine.

Next question would be, should htuple.f be removed in upstream mg5amcnlo or in the plugin... I can do both

Thanks Andrea

— Reply to this email directly, view it on GitHubhttps://github.com/madgraph5/madgraph4gpu/issues/967, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AH6535V2L7ZYQMG2APE6FKDZRRYSPAVCNFSM6AAAAABMR4LS2WVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ3DONZTGIYDGNQ. You are receiving this because you were mentioned.Message ID: @.***>

valassi commented 1 month ago

Hi Olivier, thanks.

Let's keep this open then. I think that removing this in the plugin is safe (especially if at some point we play with curand or even just vectorizing phase space the htuple.f is completely useless). But apart from developer confusion there is not much harm here... so ok if you prefer not to.

valassi commented 1 month ago

I suggest in any case to at least change this comment, otherwise it is just totally misleading for anyone reading the code

epochX/cudacpp/pp_dy3j.mad/Source/dsample.f /tmp/git-blob-CrupNA/dsample.f ffbab045d5b5f611dd06879e382399998832abd2 100644 epochX/cudacpp/pp_dy3j.mad/Source/dsample.f 0000000000000000000000000000000000000000 100644
740c740
<       data ituple/1/             !1=htuple, 2=sobel 
---
>       data ituple/1/             !1=ntuple(ranmar or htuple), 2=sobel 
oliviermattelaer commented 4 weeks ago

Done: https://github.com/mg5amcnlo/mg5amcnlo/commit/a94da7602ab1d7f8ec5f8a1f6607933d6a9f5f7c