ZDCFitWrapper: Initialize constant term in PrePulse wrapper in DoInitialize()

Change from @bcole restores missing initializer in ZDCFitWrapper.cxx in ZDCFitExpFermiPrePulse::DoInitialize. I ran @christos setup from https://its.cern.ch/jira/browse/ATLASRECTS-7759 twice in two separate directories (just changing the raw data to a Minbias file from Run 461681). I also ran his "acmd diff-root" script (as given) on the two files and I see no change.

Py:diff-root         INFO comparing over [100] entries...
Py:diff-root         INFO comparing [1138] leaves over entries...
Py:diff-root         INFO Found [2204448] identical leaves
Py:diff-root         INFO Found [0] different leaves
Py:diff-root         INFO all good.

I would appreciate a cross check of this, but this looks promising. Scripts and output can be found here: /eos/atlas/atlascerngroupdisk/det-zdc/norepro/

This merge request affects 1 package:

• ForwardDetectors/ZDC/ZdcAnalysis

This merge request affects 1 file:

• ForwardDetectors/ZDC/ZdcAnalysis/Root/ZDCFitWrapper.cxx

added 23.0 ForwardDetectors analysis-review-required review-pending-level-1 labels

changed title from Initialize constant term in PrePulse wrapper in DoInitialize() to ZDCFitWrapper: Initialize constant term in PrePulse wrapper in DoInitialize()

removed 23.0 ForwardDetectors analysis-review-required review-pending-level-1 labels

changed the description

added 23.0 ForwardDetectors bugfix review-approved labels

removed review-approved label

added review-pending-level-1 label

removed bugfix label

added bugfix label

Will try the input from the JIRA some time tomorrow in case no-one beats me to it.

As the issue with the irreproducible is that they can be annoying to reproduce .

Anyhow the change seems fine code wise .

removed review-pending-level-1 label

added review-approved label

The system selected 45 tests to probe the Athena changeset (out of 70 available tests). Link to tests selection rules

CI Result SUCCESS (hash 0877b516)

	Athena	AnalysisBase	AthAnalysis
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 78794]

resolved all threads

resolved all threads

Hi @steinber,

how many threads did you use for your reproducibility test? I tested the combination of this change + @christos' MR !66182 (closed) using 8 threads on data23_hi.00461669.express_express.merge.RAW._lb0713._SFO-ALL._0001.1

I still see the same irreproducibility.

• Walter

Hi

• This fix will prb go in

• My MR on threads will not help on this... It tries to exclude one possible issue. But this is not the issue we seem to have. i.e does not matter if you add it or not you still see the issue. We do not seem to have concurent threads competing for the variables.

• The issue seems to relate to state. But state that seems to come from what the fit did before.

• The results of fits change but printing it seems to happen in a subtle way like something is not reset between sub-sequent fits.

I think we can continue in the JIRA but one hint given what I see with full print outs, is that the problem could relate to what exactly happens on failed fits.

Examples

INFO 083.0.ZdcSumsAuxDyn.ModuleMask.083.0.ZdcSumsAuxDyn.ModuleMask 15 -> 13 => diff= [3.57142857%]

NFO 043.1.ZdcSumsAuxDyn.ModuleMask.044.1.ZdcSumsAuxDyn.ModuleMask 7 -> 15 => diff= [-18.18181818%]

Actually this might explain a bit how/when is seen . Somehow bit 2 and 4 "change" and I assume to trigger this we need a failed fit to have occured ...

@christos: The mask isn't necessarily a failure - a missing bit can also indicate a small pulse. @wlampl: I was running exactly what @christos put into the JIRA, the only thing I changed was the file (to Minbias stream, for no specific reason but I wanted to see what happened, and a I assumed wrongly that it would be similar everywhere). I will check the original file now.

Hi,

I can believe that min bias will succeed and other input can fail. But then this might be an actual good hint.

It might be that something is set when the pulse is small or the fit fails or any special situation which I would not know (I just looked the 2 bits)

And then this "special" situation might not happen in 100 minbias events but can happen 2-3 times in 100 events of the other input?

PS : As I said my MT MR does not change something. I think it does not break something, but it does not help with what wesee. So is not a "race" .

seems more like a not complete clean up between fits or something subtle like this ...

I just did a full reprocessing of the two sets (express from @christos and minbias chosen by me) with the MR branch and i reconfirm that 1) 100 minbias events agree perfectly, while the express file has a substantial set of mismatches (but the deltas are generally quite small). We'll keep investigating.

added 1 commit

• 254c784e - restored doCalib=True for PbPb2023

Compare with previous version

This merge request affects 2 packages:

• ForwardDetectors/ZDC/ZdcAnalysis
• ForwardDetectors/ZDC/ZdcRec

Affected files list will not be printed in this case

added analysis-review-required review-pending-level-1 labels and removed review-approved label

Just FYI: I have pushed a commit to our ZdcRecConfig (CA configuration) - i was out of date with the Tier0 version, which applies the energy calibration. Maybe i'm being overly optimistic but i hope we can get this in tonight.

removed analysis-review-required label

I remove the analysis as this is unrelated wrt to tier-0

I think is up to the PROC to put this in .

so should i remove? i can make a separate one - it's just to avoid further delay.

The question would be since

    # disabling this since Tier0 has no permissions
    #
    #if (doCalib == True):
    #    makeCalib(flags.Input.RunNumber[0])

do we really need this

import ROOT
from ROOT import gROOT
import os
import shutil

def makeCalib( run ):
    fname = 'ZdcCalib_Run%s.root' % run
    if (not os.path.isfile('/eos/atlas/atlascerngroupdisk/asg-calib/ZdcAnalysis/%s' % fname)):
        print('ZdRecConfig: Generating nominal calibration %s' % fname)
        gROOT.Macro('/eos/atlas/atlascerngroupdisk/det-zdc/ZDC-Calibration-Transformations/ZDC_fake_calib.C(%s)' % run)
        if (os.path.isfile(fname)):
            print('Installing %s in ZdcAnalysis group area' % fname)
            shutil.move(fname,'/eos/atlas/atlascerngroupdisk/asg-calib/ZdcAnalysis/')
        else:
            print('ZdcRecConfig.py: no file %s created!' % fname)
    else:
        print('ZdcRecConfig.py: calibration file %s exists already' % fname)

if the function is not used why introduce it here at least?

The rest of the changes are clear to me

In fact, no. Will remove.

The system selected 45 tests to probe the Athena changeset (out of 70 available tests). Link to tests selection rules

added 1 commit

• 85b9917b - removed obsolete makeCalib function

Compare with previous version

This merge request affects 2 packages:

• ForwardDetectors/ZDC/ZdcAnalysis
• ForwardDetectors/ZDC/ZdcRec

Affected files list will not be printed in this case

added analysis-review-required label

CI Result SUCCESS (hash 254c784e)

	Athena	AnalysisBase	AthAnalysis
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 1
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 78834]

Please remove these (that cause the warning)

/var/lib/jenkins/workspace/CI-MERGE-REQUEST-CC7/23.0/ForwardDetectors/ZDC/ZdcRec/python/ZdcRecConfig.py:22:1: warning: F401 'ROOT' imported but unused
import ROOT

This is from the previous commit , in the new one will be a bit more so rm them all

then will approve . As better to have this in before we continue hunting I guess...

added analysis-review-approved label

removed analysis-review-required label

@jmaurer if we get the last fix , aka no "warning" from unused import lets merge tonight as even if not fixing things 100% should be at minimum one less thing to worry about.

added 1 commit

• 74015e74 - removed unneeded includes

Compare with previous version

This merge request affects 2 packages:

• ForwardDetectors/ZDC/ZdcAnalysis
• ForwardDetectors/ZDC/ZdcRec

Affected files list will not be printed in this case

added urgent label

added review-approved-point1 label

The system selected 45 tests to probe the Athena changeset (out of 70 available tests). Link to tests selection rules

CI Result SUCCESS (hash 85b9917b)

	Athena	AnalysisBase	AthAnalysis
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 1
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 78836]

The system selected 45 tests to probe the Athena changeset (out of 70 available tests). Link to tests selection rules

CI Result SUCCESS (hash 74015e74)

	Athena	AnalysisBase	AthAnalysis
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 78838]

removed review-pending-level-1 label