Different order with/without me exporter

valassi commented 2 years ago

Hi Olivier, this is really a minor issue, but I still find it useful to fix to see things more clearly. (By the way I open a bug here rather than in launchpad, as it is is all about the 311 branch anyway).

I am comparing the directories created with

generate g g > t t~
output madevent CODEGEN_mad_gg_tt --vector_size=16 --me_exporter=standalone_cudacpp

and

generate g g > t t~
output madevent CODEGEN_mad_gg_tt --vector_size=16

I am doing this also to understand the makefiles and how to build the former (which I guess is our ultimate target).

I see that essentially adding me_exporter adds all of the files from my cudacpp plugin, good (and there are one makefile and one Makefile, interesting, one lowercase and one uppercase).

Anyway, the "problem" (really cosmetics, nothing functional, but it makes differences more complicated) is that the fortran files that are generated are not reproducible. I guess it is a problem similar to https://bugs.launchpad.net/mg5amcnlo/+bug/1944444 that you can fix in similar ways?

See here (mad is madeevent+cudacpp, madonly is without cudacpp)

diff -r gg_tt.madonly/Source/MODEL/param_write.inc gg_tt.mad/Source/MODEL/param_write.inc
7a8,12
>       WRITE(*,*) 'mdl_MB = ', MDL_MB
>       WRITE(*,*) 'mdl_MT = ', MDL_MT
>       WRITE(*,*) 'mdl_MTA = ', MDL_MTA
>       WRITE(*,*) 'mdl_MZ = ', MDL_MZ
>       WRITE(*,*) 'mdl_MH = ', MDL_MH
14,18c19
<       WRITE(*,*) 'mdl_MZ = ', MDL_MZ
<       WRITE(*,*) 'mdl_MT = ', MDL_MT
<       WRITE(*,*) 'mdl_MB = ', MDL_MB
<       WRITE(*,*) 'mdl_MH = ', MDL_MH
<       WRITE(*,*) 'mdl_MTA = ', MDL_MTA
---
>       WRITE(*,*) 'mdl_WT = ', MDL_WT
21d21
<       WRITE(*,*) 'mdl_WT = ', MDL_WT

Thanks! Andrea

valassi commented 2 years ago

The previous bug https://bugs.launchpad.net/mg5amcnlo/+bug/1944444 was fixed in https://bazaar.launchpad.net/~maddevelopers/mg5amcnlo/2.7.0_gpu/revision/368 https://bazaar.launchpad.net/~maddevelopers/mg5amcnlo/2.7.0_gpu/revision/369 https://bazaar.launchpad.net/~maddevelopers/mg5amcnlo/2.7.0_gpu/revision/370 I am having a look to see if I find a quick solution to this one too

valassi commented 2 years ago

For my understanding, note that debug is a python constant that is always true unless python is started with -o (?!): https://docs.python.org/3/library/constants.html This means that madgraph.ordering should always be true (indeed bth my mad and madonly logfiles start with "Running MG5 in debug mode")

valassi commented 2 years ago

Note that I am using python 3.8.6 and dictionaries are insertion-ordered (hence reproducible, in principle) as of python 3.7 https://stackoverflow.com/questions/39980323/are-dictionaries-ordered-in-python-3-6

Note also that the original problem was using set. This was my patch (not merged, but Olivier's patch is similar, and more complete) https://bazaar.launchpad.net/~andrea-valassi/mg5amcnlo/2.7.0_gpu_patches/revision/368

valassi commented 2 years ago

Ouf. This was tough to debug, and I just started.

The difference between the two modes (with/without me exporter) comes from this line https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/madgraph/interface/madgraph_interface.py#L8233

        if options['output'] == 'Template':
            self._curr_exporter.copy_template(self._curr_model)
            if self._me_curr_exporter:
                self._me_curr_exporter.copy_template(self._curr_model)

Essentially, the model in self._curr_model is changed by "self._me_curr_exporter.copy_template(self._curr_model)".

This silly debugging code shows it

        if options['output'] == 'Template':
            self._curr_exporter.copy_template(self._curr_model)
            for key in self._curr_model['parameters'].keys():
                print ("HALLOMG-2ccc", key, [o.name for o in self._curr_model['parameters'][key] if o.name])
            if self._me_curr_exporter:
                self._me_curr_exporter.copy_template(self._curr_model)
            for key in self._curr_model['parameters'].keys():
                print ("HALLOMG-2ddd", key, [o.name for o in self._curr_model['parameters'][key] if o.name])

it gives in both cases

HALLOMG-2ccc ('external',) ['aEWM1', 'mdl_Gf', 'aS', 'mdl_ymb', 'mdl_ymt', 'mdl_ymtau', 'mdl_MZ', 'mdl_MT', 'mdl_MB', 'mdl_MH', 'mdl_MTA', 'mdl_WZ', 'mdl_WW', 'mdl_WT', 'mdl_WH']

but it differes here

< HALLOMG-2ddd ('external',) ['mdl_MB', 'mdl_MT', 'mdl_MTA', 'mdl_MZ', 'mdl_MH', 'aEWM1', 'mdl_Gf', 'aS', 'mdl_ymb', 'mdl_ymt', 'mdl_ymtau', 'mdl_WT', 'mdl_WZ', 'mdl_WW', 'mdl_WH']
> HALLOMG-2ddd ('external',) ['aEWM1', 'mdl_Gf', 'aS', 'mdl_ymb', 'mdl_ymt', 'mdl_ymtau', 'mdl_MZ', 'mdl_MT', 'mdl_MB', 'mdl_MH', 'mdl_MTA', 'mdl_WZ', 'mdl_WW', 'mdl_WT', 'mdl_WH']

Even weirder, the curr_model in the "finalize" method, which is after all of this, is again the same in both cases.

I have a hard time understanding

why the curr model keeps being modified by various methods
also, why the me exporter needs its own model: I mean, whether we use Fortran or C++ for computing MEs, the underlying physics model should be the same? I guess that "model" here is much more than a physics model, but also contains some language-specific features?

It may also be that this is my fault, frrom https://github.com/madgraph5/madgraph4gpu/issues/341 ... but I cannot judge this

I will continue having a look (it is useful to debug issue 341 anyway)

valassi commented 2 years ago

It may also be that this is my fault, frrom madgraph5/madgraph4gpu#341 ... but I cannot judge this

NO. At least I can exclude this. The same issue happens if I compare

output madevent CODEGEN_mad_gg_tt --vector_size=16 --me_exporter=standalone_gpu
output madevent CODEGEN_mad_gg_tt --vector_size=16

In both cases, I do NOT copy my cudacpp plugin, so the problem is completely internal to the upstream madgraph

valassi commented 2 years ago

@oliviermattelaer to rephrase all of this: if curr_model needs to be different for the "main" exporter and the "me exporter", is it not better to have two different self._curr_model and self._curr_me_model?

valassi commented 2 years ago

SO.... TO CUT A LONG STORY SHORT: this is the culprit https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/models/write_param_card.py#L204

    def write_card(self, path=None, write_special=True):
        """schedular for writing a card"""
        if path:
            self.define_input_file(path)
        # order the parameter in a smart way
        self.external.sort(key=cmp_to_key(self.order_param))

To elaborate a bit: the REAL problem is that the whole python machinery in mg5amc does not clearly distinguish inputs, outputs, states, constness... This is actually because it is very difficult to do this in python, as python is dunamically typed. Essentially, most assignments are assignments of references, rather than deep copies, and what might seem like a totally innocent change on a local variable ends up modifying a variable that is referenced throughout the code.

In this case, this is the calling sequence https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/madgraph/interface/madgraph_interface.py#L8233 calls https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/madgraph/iolibs/export_cpp.py#L2542 calls https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/madgraph/core/base_objects.py#L1641 which copies model by reference here https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/models/write_param_card.py#L75 and eventually modifies the model here https://github.com/oliviermattelaer/mg5amc_test/blob/64bc9caa020e23a708fc4aa7e3b2b00306bed2ba/models/write_param_card.py#L204

How to fix this? I guess there are several ways.

Is that sort needed at all or can it be removed?
Can the reference copy become a deep copy, or is it necessary to modify model as a reference?
Should there be two models for the main eporter and the me exporter?
... and possibly more @oliviermattelaer up to you to decide if you want to fix this and how...!

I guess that for the moment I will maybe add a deep copy and see. I just to make sure that I get the same code in the fortran part, except for the cuda/cpp additions, to ease debugging.

I ededn up spending far too much time on this, but at least I got a slightly better idea how the rest of the python code works.

oliviermattelaer commented 2 years ago

Hi,

A deep copy is irrelevant here, since the ordering of the external parameter are really physically irrelevant. The fact that i sort them at one place is to have some nicer output at that point. Since this is not a cpu intensive task, I have force to re-run that sorting before writing that specific file to be sure that such file has the same ordering (even if it is less critical for that output).

This will be actually cool for some automatic test where this issue was making some automatic test to fail.

valassi commented 2 years ago

Hi @oliviermattelaer sorry I am reopening this issue (but feel free to close it again!). It does not appear fixed for me, I still need to use my workaround with a deep copy.

Is this because

you think it does not make sense to fix this, so the two outputs should be different?
you think you have fixed it? (but then I am not sure why it does not work for me)

I am using revision 999, which in principle includes your fix in https://bazaar.launchpad.net/~maddevelopers/mg5amcnlo/3.1.1_lo_vectorization/revision/998

Thanks! Andrea

valassi commented 2 years ago

OOOPS! Sorry, I spoke too early. Your fix is in export_v4.py, which I was overloading as a workaround for another issue #6

I will try again...

valassi commented 2 years ago

Hi @oliviermattelaer again, I confirm that there is still a difference.

diff -r gg_tt.madonly/Source/ gg_tt.mad/Source/
diff -r gg_tt.madonly/Source/param_card.inc gg_tt.mad/Source/param_card.inc
0a1,5
>       MDL_MB = 4.700000D+00
>       MDL_MT = 1.730000D+02
>       MDL_MTA = 1.777000D+00
>       MDL_MZ = 9.118800D+01
>       MDL_MH = 1.250000D+02
7,11c12
<       MDL_MZ = 9.118800D+01
<       MDL_MT = 1.730000D+02
<       MDL_MB = 4.700000D+00
<       MDL_MH = 1.250000D+02
<       MDL_MTA = 1.777000D+00
---
>       MDL_WT = 1.491500D+00
14d14
<       MDL_WT = 1.491500D+00

So, param_card.inc differes; instead param_write.inc (the one I had reported initially about) is now fixed by your fix. I guess this makes no difference in any case? Just to ease my bookkeeping an dchecks, I will maybe reorder param_card.inc when generating the processes, so as to ease comparisons.

I keep this open as a reminder for you: please have a look, and decide if it needs a further fix or can just be ignored. Thanks! Andrea

valassi commented 2 years ago

PS Of course, that difference comes from this

diff -r gg_tt.madonly/Cards/ident_card.dat gg_tt.mad/Cards/ident_card.dat
4a5,14
> mass 5 mdl_MB
> 
> mass 6 mdl_MT
> 
> mass 15 mdl_MTA
> 
> mass 23 mdl_MZ
> 
> mass 25 mdl_MH
> 
17,25c27
< mass 23 mdl_MZ
< 
< mass 6 mdl_MT
< 
< mass 5 mdl_MB
< 
< mass 25 mdl_MH
< 
< mass 15 mdl_MTA
---
> decay 6 mdl_WT

I will reorder this dat file during generation in my own scripts

valassi commented 2 years ago

Hm, worse, I was forgetting that I also get this:

diff -r gg_tt/src/Parameters_sm.cc gg_tt.mad/src/Parameters_sm.cc
34d33
<   mdl_WT = slha.get_block_entry( "decay", 6, 1.491500e+00 );
37,41c36
<   mdl_MTA = slha.get_block_entry( "mass", 15, 1.777000e+00 );
<   mdl_MH = slha.get_block_entry( "mass", 25, 1.250000e+02 );
<   mdl_MB = slha.get_block_entry( "mass", 5, 4.700000e+00 );
<   mdl_MT = slha.get_block_entry( "mass", 6, 1.730000e+02 );
<   mdl_MZ = slha.get_block_entry( "mass", 23, 9.118800e+01 );
---
>   mdl_WT = slha.get_block_entry( "decay", 6, 1.491500e+00 );
47a43,47
>   mdl_MH = slha.get_block_entry( "mass", 25, 1.250000e+02 );
>   mdl_MZ = slha.get_block_entry( "mass", 23, 9.118800e+01 );
>   mdl_MTA = slha.get_block_entry( "mass", 15, 1.777000e+00 );
>   mdl_MT = slha.get_block_entry( "mass", 6, 1.730000e+02 );
>   mdl_MB = slha.get_block_entry( "mass", 5, 4.700000e+00 );

I know I am picky, but I would like to be able to simply check that the code is the same in fortran, cudacpp, fortran+cudacpp (where some pieces of code are common, and unless there are good reasons for differences). My "deep copy" patch was achieving that. I will keep checking...

oliviermattelaer commented 2 years ago

Hi,

I really dislike the deep-copy since this can lead to difference between the code. They are no reason to have two model here. It is more dangerous than anything else. (if we need to change something into the model --like adding a lorentz structure--, it is important that the change applies for both)

Now, all the differences above have zero relevance (neither in term of physics nor in term of performance).

To me, this is only a matter of convenience. Since ordering such list is certainly negligible in term of cpu, we can certainly apply an ordering function to ease such comparison. Would that work for you?

Olivier

valassi commented 2 years ago

Hm, worse, I was forgetting that I also get this:

diff -r gg_tt/src/Parameters_sm.cc gg_tt.mad/src/Parameters_sm.cc
34d33
<   mdl_WT = slha.get_block_entry( "decay", 6, 1.491500e+00 );
37,41c36
<   mdl_MTA = slha.get_block_entry( "mass", 15, 1.777000e+00 );
<   mdl_MH = slha.get_block_entry( "mass", 25, 1.250000e+02 );
<   mdl_MB = slha.get_block_entry( "mass", 5, 4.700000e+00 );
<   mdl_MT = slha.get_block_entry( "mass", 6, 1.730000e+02 );
<   mdl_MZ = slha.get_block_entry( "mass", 23, 9.118800e+01 );
---
>   mdl_WT = slha.get_block_entry( "decay", 6, 1.491500e+00 );
47a43,47
>   mdl_MH = slha.get_block_entry( "mass", 25, 1.250000e+02 );
>   mdl_MZ = slha.get_block_entry( "mass", 23, 9.118800e+01 );
>   mdl_MTA = slha.get_block_entry( "mass", 15, 1.777000e+00 );
>   mdl_MT = slha.get_block_entry( "mass", 6, 1.730000e+02 );
>   mdl_MB = slha.get_block_entry( "mass", 5, 4.700000e+00 );

I know I am picky, but I would like to be able to simply check that the code is the same in fortran, cudacpp, fortran+cudacpp (where some pieces of code are common, and unless there are good reasons for differences). My "deep copy" patch was achieving that. I will keep checking...

Hi Olivier, sorry, my fault, I am being very messy these days - many things. In particular, I made a mess of my own conventions, "gg_tt" is the stuff I change manually, while "gg_tt.auto" is the stuff I generate from standalone cudacpp. I generally resync the former from the latter after regenerating, but I forgot. So all that I said in the snippet above is wrong. There are no such differences left.

I did indeed worka around small residual differences by sorting the ident_card https://github.com/madgraph5/madgraph4gpu/blob/c260c7cc8f2852aaad87f98d4519f5eac3c9014e/epochX/cudacpp/CODEGEN/generateAndCompare.sh#L134

# Add a workaround for https://github.com/oliviermattelaer/mg5amc_test/issues/2
  if [ "${OUTBCK}" == "madonly" ] || [ "${OUTBCK}" == "mad" ] || [ "${OUTBCK}" == "madcpp" ] || [ "${OUTBCK}" == "madgpu" ]; then
    cat ${OUTDIR}/${proc}.${autosuffix}/Cards/ident_card.dat | head -3 > ${OUTDIR}/${proc}.${autosuffix}/Cards/ident_card.dat.new
    cat ${OUTDIR}/${proc}.${autosuffix}/Cards/ident_card.dat | tail -n+4 | sort >> ${OUTDIR}/${proc}.${autosuffix}/Cards/ident_card.dat.new
    \mv ${OUTDIR}/${proc}.${autosuffix}/Cards/ident_card.dat.new ${OUTDIR}/${proc}.${autosuffix}/Cards/ident_card.dat
  fi

All issues are fixed. (And I no longer overwrite your export_v4.py... still https://github.com/madgraph5/madgraph4gpu/issues/341 to fix but that's another story).

This is indeed closed.

oliviermattelaer / mg5amc_test

Different order with/without me exporter #2