mg5amcnlo / mg5amcnlo

Other
65 stars 35 forks source link

master_june24 completion: nb_warp_used (and a SUBset of gpucpp_june24) into gpucpp #121

Closed valassi closed 2 months ago

valassi commented 4 months ago

Hi @oliviermattelaer this is a WIP MR into gpucpp of my nb_warp_used and of everything else I am using against master_june24 in my (https://github.com/madgraph5/madgraph4gpu/pull/882).

The nb_warp_used fixes are described here https://github.com/madgraph5/madgraph4gpu/issues/885#issuecomment-2233827188

NB: as discussed in https://github.com/madgraph5/madgraph4gpu/issues/886, gpucpp_june24, with or without my changes, does NOT work for me with master_june24 (my modified version, but I am pretty sure that the problem was pre-existing). This is why here I am NOT merging gpucpp_june24, I am only merging a SUBSET of that (what was originally used in master_june24, plus my changes, see https://github.com/valassi/mg5amcnlo/commits/valassi_gpucpp_june24/

I keep this in WIP for the moment. I would only merge this when we agree to merge my https://github.com/madgraph5/madgraph4gpu/pull/882 into master (as master is meant to use the head of gpucpp... I do not want to 'pollute' gpucpp before!).

I also filed PR #120 against gpucpp_june24, but as mentioned in https://github.com/madgraph5/madgraph4gpu/issues/886, I am unable to use gpucpp_june24 and this is NOT what was used in master_june24 anyway.

Thanks Andrea

valassi commented 3 months ago

Hi @oliviermattelaer this is a preliminary step for merging the master_june24 work of https://github.com/madgraph5/madgraph4gpu/pull/882

Can you please review and approve?

Note, this is related to https://github.com/madgraph5/madgraph4gpu/issues/886: in practice, what is here my valassi_gpucpp_june24 gets merged into gpucpp, while we forget about gpucpp_june24.

Thanks! Andrea

valassi commented 3 months ago

PS @oliviermattelaer PLEASE DO NOT SQUASH in case you merge this, thanks :-)

valassi commented 2 months ago

Hi @oliviermattelaer I just merged th elatest gpucpp (including your gpucpp_fixci #132) into my valassi_gpucpp_june24.

I will update also my https://github.com/madgraph5/madgraph4gpu/pull/882 in madgraph4gpu to use this one.

(Then I will probably comment on your https://github.com/madgraph5/madgraph4gpu/pull/981 that 981 becomes irrelevant)

valassi commented 2 months ago

Hi @oliviermattelaer this now includes everything we need for june24. Can you have a look and approve? Thanks Andrea PS Again please do not squash this...

oliviermattelaer commented 2 months ago

Looks like they are conflict to fix here. Additionally don't you want to use nb_warp_used? in the file dsample.f Or do you prefere anoter PR for that?

Olivier

valassi commented 2 months ago

Hi Olivier thanks

Looks like they are conflict to fix here.

Hm? No I do not see conflicts, which conflicts are you talking about?

Additionally don't you want to use nb_warp_used? in the file dsample.f Or do you prefere anoter PR for that?

No I wanted to fix this here.

I am fixing it in various places. See https://github.com/mg5amcnlo/mg5amcnlo/pull/121/files#r1740773122

I thought that this was enough because tests were passing. But you are right that I may be missing some and that these go undetected. Ok then lets put this back to WIP :-(

Andrea

oliviermattelaer commented 2 months ago
Screenshot 2024-09-02 at 13 56 21

Ok looks like this is another subtelty of github, that I had to understand... But this is only because the button was on "rebase" if I move it to "merge" then github does not complain about conflict anymore ... Sorry for the confusion

valassi commented 2 months ago

Additionally don't you want to use nb_warp_used? in the file dsample.f

Hi @oliviermattelaer again. I had a look at this, see the comments in https://github.com/madgraph5/madgraph4gpu/issues/983. In my opinion there is nothing else to be done in dsample.f, but I may be missing something. Can you have a look yourself and comment in 983 please?

Then if we agree that there is nothing else to do , I think that we can start merging this.

Thanks Andrea

valassi commented 2 months ago

Hi @oliviermattelaer again. I had a look at this, see the comments in madgraph5/madgraph4gpu#983. In my opinion there is nothing else to be done in dsample.f, but I may be missing something. Can you have a look yourself and comment in 983 please?

Hi @oliviermattelaer thanks for having a look! You confirmed here https://github.com/madgraph5/madgraph4gpu/issues/983#issuecomment-2325232569 that there is nothing to do for 983, so I consider that this is good to go.

So, I AM MERGING THIS! Starting the full june24 merge into master+gpucpp...