TPC: adding option to use custom scalers (IDCs) for SC correction map scaling by matthias-kleiner · Pull Request #12205 · AliceO2Group/AliceO2

matthias-kleiner · 2023-11-04T17:39:21Z

Adding a generic method to provide scaling of the space-charge correction maps based on the IDCs (and cluster currents). In the o2-tpc-scaler-workflow for each TF the last n values (dependent on the specified ion drift time and integration length of the scalers) are averaged and provided as a scaling value.

A- and C-side values are averaged for now!

Example how to run with scalers from local file:

  scaler="file:///data/mkleiner/run3/LHC23zo/539399_lowIR/=TPC/Calib/Scaler"

  o2-ctf-reader-workflow --copy-cmd no-copy --onlyDet TPC --ctf-input ${INPUT} --severity warning --condition-remap "${scaler}" |
  o2-tpc-scaler-workflow |
  o2-tpc-reco-workflow --lumi-type 2 --input-type compressed-clusters-ctf --output-type "tracks,disable-writer" --disable-mc -b

matthias-kleiner · 2023-11-04T17:46:51Z

Hello @shahor02 @wiechula
can you take a look if this implementation of providing the IDCs as a scaling for the correction maps is fine for you? I tested it locally and it seemed reasonable for me to put it in an own device

shahor02

Looks good, please see a few minor comments below.

Also,

AliceO2/prodtests/full-system-test/dpl-workflow.sh

Lines 269 to 277 in ccbd217

    
           ASK_CTP_LUMI_GPU="--require-ctp-lumi" 
        
           : ${TPC_CORR_SCALING_GPU:=""} 
        
           if [[ "$TPC_CORR_SCALING" == *"$ASK_CTP_LUMI_GPU"* ]]; then 
        
               TPC_CORR_SCALING_GPU=${TPC_CORR_SCALING//$ASK_CTP_LUMI_GPU/} 
        
           else 
        
             ASK_CTP_LUMI_GPU="" 
        
             TPC_CORR_SCALING_GPU="$TPC_CORR_SCALING" 
        
           fi 
        
           GPU_CONFIG_SELF+=" $TPC_CORR_SCALING_GPU "

will need to be modified: the problem is that the --require-ctp-lumi (and now your lumi-type) are workflow level options while the rest is device level options.
When the same device needs to use severity different from the default, we need to pass this option as e.g. --gpu-reconstruction \"<option>\" and now the options must include all options of that device...
Could you find a way to remove from the $TPC_CORR_SCALING the eventual lumi-type <N> and assign it to ASK_CTP_LUMI_GPU?

shahor02 · 2023-11-04T18:33:21Z

Detectors/TPC/calibration/src/CorrectionMapsLoader.cxx

    setInstLumi(mInstLumiFactor * (mCTPLumiSource == 0 ? lumiObj.getLumi() : lumiObj.getLumiAlt()));
+  } else if (getLumiScaleType() == 2 && mInstLumiOverride <= 0.) {
+    float tpcScaler = pc.inputs().get<float>("tpcscaler");
+    setInstLumi(tpcScaler);


Why not applying the optional mInstLumiFactor also here? This would allow to use different maps with "mean" lumi nominated in other units than the source of "inst" lumi.

shahor02 · 2023-11-04T18:46:01Z

Detectors/TPC/calibration/src/TPCScaler.cxx

+  tpcScalerTree.SetBranchAddress("TPCScaler", &scalerTmp);
+  const int entries = tpcScalerTree.GetEntries();
+  if (entries > 0) {
+    tpcScalerTree.GetEntry(0);
+    *this = std::move(*scalerTmp);
+    delete scalerTmp;
+  } else {
+    LOGP(error, "TPCScaler not found in input file");
+  }


why not simply

tpcScalerTree.SetBranchAddress("TPCScaler", &this); if (entries > 0) { tpcScalerTree.GetEntry(0); } else { LOGP(error, "TPCScaler not found in input file"); }

?

I tried it and it seems to be not working: lvalue required as unary '&' operand. I think its related to this discussion https://stackoverflow.com/questions/13476879/how-can-you-assign-a-value-to-the-pointer-this-in-c

If I try something like this tpcScalerTree.SetBranchAddress("TPCScaler", &(*this)); it compiles, but then crashes Error in <TTree::SetBranchAddress>: The address for "TPCScaler" should be the address of a pointer!

Indeed, but you can use intermediate pointer, thus avoiding the copy of the object. For me this works:

$ cat tcl.C #include "TTree.h" #include "TFile.h" struct TstCl { int a=0; void dumpToFile(const char* file = "tstcl.root", const char* name = "tsttree") { TFile out(file, "RECREATE"); TTree tree("tsttree", ""); tree.Branch("b", this); tree.Fill(); out.WriteObject(&tree, name); } void setFromTree(TTree& t) { TstCl* adr = this; t.SetBranchAddress("b",&adr); t.GetEntry(0); t.SetBranchAddress("b",nullptr); } ClassDefNV(TstCl, 1); };

root [0] .L tcl.C+ root [1] TstCl v0{10}; root [2] v0.dumpToFile(); root [3] ff = TFile::Open("tstcl.root"); root [4] auto tt = (TTree*)gFile->Get("tsttree"); root [5] TstCl v1; root [6] v1.a (int) 0 root [7] v1.setFromTree(*tt); root [7] v1.a (int) 10

matthias-kleiner · 2023-11-07T08:51:22Z

Looks good, please see a few minor comments below.

Also,

AliceO2/prodtests/full-system-test/dpl-workflow.sh

Lines 269 to 277 in ccbd217

ASK_CTP_LUMI_GPU="--require-ctp-lumi"

: ${TPC_CORR_SCALING_GPU:=""}

if [[ "$TPC_CORR_SCALING" == *"$ASK_CTP_LUMI_GPU"* ]]; then

TPC_CORR_SCALING_GPU=${TPC_CORR_SCALING//$ASK_CTP_LUMI_GPU/}

else

ASK_CTP_LUMI_GPU=""

TPC_CORR_SCALING_GPU="$TPC_CORR_SCALING"

fi

GPU_CONFIG_SELF+=" $TPC_CORR_SCALING_GPU "

will need to be modified: the problem is that the --require-ctp-lumi (and now your lumi-type) are workflow level options while the rest is device level options.
When the same device needs to use severity different from the default, we need to pass this option as e.g. --gpu-reconstruction \"<option>\" and now the options must include all options of that device...
Could you find a way to remove from the $TPC_CORR_SCALING the eventual lumi-type <N> and assign it to ASK_CTP_LUMI_GPU?

Hello @shahor02 thanks for the comments, but I don't understand really understand what you mean by this. Can you explain it in more detail?

shahor02 · 2023-11-07T10:14:18Z

Can you explain it in more detail?

Workflow global options (those which alter inputs/outputs must be global ones, i.e. added via customize(std::vector<ConfigParamSpec>& workflowOptions) ) are added directly to the workflow command line. Device options (added via DataProcessorSpec) in general should be added to the command line as --<device_name> "<opt1> <opt2> ..." but putting then directly on the command line is still accepted provided there are no other options passed via --<device_name> " ... " .
The --require-ctp-lumi (and now your --lumi-type) are global options, all other options defined in

AliceO2/Detectors/TPC/calibration/src/CorrectionMapsLoader.cxx

Lines 88 to 92 in ccbd217

    
           addOption(options, ConfigParamSpec{"corrmap-lumi-mean", VariantType::Float, 0.f, {"override TPC corr.map mean lumi (if > 0), disable corrections if < 0"}}); 
        
           addOption(options, ConfigParamSpec{"corrmap-lumi-inst", VariantType::Float, 0.f, {"override instantaneous CTP lumi (if > 0) for TPC corr.map scaling, disable corrections if < 0"}}); 
        
           addOption(options, ConfigParamSpec{"corrmap-lumi-mode", VariantType::Int, 0, {"scaling mode: (default) 0 = static + scale * full; 1 = full + scale * derivative"}}); 
        
           addOption(options, ConfigParamSpec{"ctp-lumi-factor", VariantType::Float, 1.0f, {"scaling to apply to instantaneous lumi from CTP (but not corrmap-lumi-inst)"}}); 
        
           addOption(options, ConfigParamSpec{"ctp-lumi-source", VariantType::Int, 0, {"CTP lumi source: 0 = LumiInfo.getLumi(), 1 = LumiInfo.getLumiAlt()"}});

are device options. Since all scaling options are provided via env. var. TPC_CORR_SCALING and the gpu-reco workflow need to use explicitly the --gpu-reconstruction ... device options, we need to parse the $TPC_CORR_SCALING to 2 pieces which are passed separately.
E.g. TPC_CORR_SCALING=" --lumi-type 2 --corrmap-lumi-mean XXX" in the generated command line should become

o2-gpu-reco-workflow  --lumi-type 2 --gpu-reconstruction "--severity info  --corrmap-lumi-mean XXX "  ...

in the

o2-gpu-reco-workflow  --lumi-type 2 -- XXX --gpu-reconstruction "--severity info "  ...

the corrmap-lumi-mean XXX would be ignored due to the presence of the explicit --gpu-reconstruction.

matthias-kleiner · 2023-11-10T11:12:20Z

Many thanks @shahor02 ! If I understand correctly this should be sufficient ?:

# example
TPC_CORR_SCALING="--option 1 --lumi-type 2 --option 2"

ASK_CTP_LUMI_GPU="--require-ctp-lumi"

# if lumi-type is provided extract it from TPC_CORR_SCALING and append it to ASK_CTP_LUMI_GPU
if [[ $TPC_CORR_SCALING =~ "--lumi-type "([0-9]+) ]]; then
    lumi_type="${BASH_REMATCH[0]}"

    # Remove lumi-type from TPC_CORR_SCALING
    TPC_CORR_SCALING="${TPC_CORR_SCALING/$lumi_type}"

    # append lumi-type to ASK_CTP_LUMI_GPU
    ASK_CTP_LUMI_GPU="$ASK_CTP_LUMI_GPU $lumi_type"
fi

shahor02 · 2023-11-10T12:40:32Z

@matthias-kleiner almost: there should be no anymore require-ctp-lumi. Also, one should cover the case of the option passed as --lumi-type=x. The version below should do this and be more robust:

#!/bin/bash

echo "testing $TPC_CORR_SCALING"

parse_TPC_CORR_SCALING()
{
ASK_CTP_LUMI_GPU=
local restOpt=
while [[ $# -gt 0 ]]; do
  case "$1" in
    --lumi-type=*) ASK_CTP_LUMI_GPU=" --lumi-type ${1#*=}"; shift 1;;
    --lumi-type) ASK_CTP_LUMI_GPU=" --lumi-type ${2}"; shift 2;;
    *) restOpt+=" $1"; shift 1;;
  esac
done
TPC_CORR_SCALING=$restOpt
}

parse_TPC_CORR_SCALING $TPC_CORR_SCALING

echo ASK_CTP_LUMI_GPU=$ASK_CTP_LUMI_GPU
echo TPC_CORR_SCALING=$TPC_CORR_SCALING

matthias-kleiner · 2023-11-10T13:30:07Z

@matthias-kleiner almost: there should be no anymore require-ctp-lumi. Also, one should cover the case of the option passed as --lumi-type=x. The version below should do this and be more robust:

#!/bin/bash

echo "testing $TPC_CORR_SCALING"

parse_TPC_CORR_SCALING()
{
ASK_CTP_LUMI_GPU=
local restOpt=
while [[ $# -gt 0 ]]; do
  case "$1" in
    --lumi-type=*) ASK_CTP_LUMI_GPU=" --lumi-type ${1#*=}"; shift 1;;
    --lumi-type) ASK_CTP_LUMI_GPU=" --lumi-type ${2}"; shift 2;;
    *) restOpt+=" $1"; shift 1;;
  esac
done
TPC_CORR_SCALING=$restOpt
}

parse_TPC_CORR_SCALING $TPC_CORR_SCALING

echo ASK_CTP_LUMI_GPU=$ASK_CTP_LUMI_GPU
echo TPC_CORR_SCALING=$TPC_CORR_SCALING

okay, many thanks for your help

… scaling Adding a generic method to provide scaling of the space-charge correction maps based on the IDCs (and cluster currents). In the `o2-tpc-scaler-workflow` for each TF the last n values (dependent on the specified ion drift time and integration length of the scalers) are averaged and provided as scaling value. - A- and C-side values are averaged for now! Example how to run with scalersfrom local file: ``` scaler="file:///data/mkleiner/run3/LHC23zo/539399_lowIR/=TPC/Calib/Scaler" o2-ctf-reader-workflow --copy-cmd no-copy --onlyDet TPC --ctf-input ${INPUT} --severity warning --condition-remap "${scaler}" | o2-tpc-scaler-workflow | o2-tpc-reco-workflow --lumi-type 2 --input-type compressed-clusters-ctf --output-type "tracks,disable-writer" --disable-mc -b ```

- Adding mInstLumiFactor as optional scaling factor

shahor02

@matthias-kleiner the last comment on the setFromTree is just a suggestion (to avoid object copy of potentially large object). Otherwise, the PR looks fine, but it is still in draft mode.

matthias-kleiner · 2023-11-11T16:37:02Z

@matthias-kleiner the last comment on the setFromTree is just a suggestion (to avoid object copy of potentially large object). Otherwise, the PR looks fine, but it is still in draft mode.

Many thanks, this works now

shahor02

@wiechula is it ok for you to merge this? The CI failures are unrelated. I have a PR to open which would interfere with this PR so I better merge it before mine.

wiechula

@shahor02 looks fine for me.
@matthias-kleiner thanks for the implementation!

shahor02 reviewed Nov 4, 2023

View reviewed changes

matthias-kleiner added 4 commits November 11, 2023 12:50

Fixing setting of lumi from TPC scalers

af72fb5

Adding check for run number, adding option to set ion drift time

4be2f6c

- Adding mInstLumiFactor as optional scaling factor

Parse lumi-type from TPC_CORR_SCALING to ASK_CTP_LUMI_GPU

ce75ac5

matthias-kleiner force-pushed the DCAd_monitoring branch from ccbd217 to ce75ac5 Compare November 11, 2023 12:36

shahor02 reviewed Nov 11, 2023

View reviewed changes

Avoid copy when loading object from file

0adc103

matthias-kleiner marked this pull request as ready for review November 11, 2023 16:37

matthias-kleiner requested review from a team, davidrohr, sgorbuno and wiechula as code owners November 11, 2023 16:37

sgorbuno approved these changes Nov 12, 2023

View reviewed changes

shahor02 approved these changes Nov 12, 2023

View reviewed changes

wiechula approved these changes Nov 12, 2023

View reviewed changes

shahor02 merged commit 044a4ec into AliceO2Group:dev Nov 12, 2023

shahor02 mentioned this pull request Nov 13, 2023

Change --require-tpc-lumi option to --lumi-type <type> AliceO2Group/O2DPG#1325

Merged

	ASK_CTP_LUMI_GPU="--require-ctp-lumi"
	: ${TPC_CORR_SCALING_GPU:=""}
	if [[ "$TPC_CORR_SCALING" == "$ASK_CTP_LUMI_GPU" ]]; then
	TPC_CORR_SCALING_GPU=${TPC_CORR_SCALING//$ASK_CTP_LUMI_GPU/}
	else
	ASK_CTP_LUMI_GPU=""
	TPC_CORR_SCALING_GPU="$TPC_CORR_SCALING"
	fi
	GPU_CONFIG_SELF+=" $TPC_CORR_SCALING_GPU "

Conversation

matthias-kleiner commented Nov 4, 2023

Uh oh!

matthias-kleiner commented Nov 4, 2023

Uh oh!

shahor02 left a comment

Choose a reason for hiding this comment

Uh oh!

shahor02 Nov 4, 2023

Choose a reason for hiding this comment

Uh oh!

shahor02 Nov 4, 2023

Choose a reason for hiding this comment

Uh oh!

matthias-kleiner Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

shahor02 Nov 11, 2023

Choose a reason for hiding this comment

Uh oh!

matthias-kleiner commented Nov 7, 2023

Uh oh!

shahor02 commented Nov 7, 2023

Uh oh!

matthias-kleiner commented Nov 10, 2023

Uh oh!

shahor02 commented Nov 10, 2023

Uh oh!

matthias-kleiner commented Nov 10, 2023

Uh oh!

shahor02 left a comment

Choose a reason for hiding this comment

Uh oh!

matthias-kleiner commented Nov 11, 2023

Uh oh!

shahor02 left a comment

Choose a reason for hiding this comment

Uh oh!

wiechula left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants