CPV fix simulation workflow #5887

sevdokim · 2021-04-08T13:20:45Z

Simulation procedure adjusted: added pedestals to raw data creator, tuned simulation parameters.
Reduced noise speeds up clusterization procedure by factor ~20 times.
Added pedestal subtraction in the raw -> digits workflow.

davidrohr · 2021-04-08T19:52:16Z

thx, will check the clusterizer performance with this.

sevdokim · 2021-04-09T05:41:22Z

thx, will check the clusterizer performance with this.

Dear David, all,

unfortunately there appeared some errors during fullCI build:

sw/BUILD/o2checkcode-latest/log

--

========== List of errors found ==========
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5887/0/Detectors/CPV/simulation/src/Digitizer.cxx:80:38: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5887/0/Detectors/CPV/simulation/src/Digitizer.cxx:150:36: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5887/0/Detectors/CPV/simulation/src/Digitizer.cxx:161:39: error: statement should be inside braces [readability-braces-around-statements]
/mnt/mesos/sandbox/sandbox/o2-fullci/sw/SOURCES/O2/5887/0/Detectors/CPV/workflow/src/RawToDigitConverterSpec.cxx:195:25: error: statement should be inside braces [readability-braces-around-statements]
--

Machine think that code is not readable which I can debate. However Those errors didn't show up at my laptop. Would it be possible to explain what are those, how to reproduce them locally and how to get rid of it? I don't want to fix something blindly and wait another half day for tests.

Thank you & regards,
Sergey

davidrohr · 2021-04-09T06:45:07Z

Hi @sevdokim, these are codechecker errors, they should be quite straight forward to fix.

In general, running the codechecker locally is not completely straight forward.
The simplest way is to build inside the alisw/slc8-gpu-builder docker container, and then build the package O2FullCI instead of O2, and setting the env variable ALIBUILD_O2_TESTS=1. Then, it will build in exactly the same way as done in the CI.

You can also build it locally, but that often gives some problem with clang and system headers, and there is no good recipe to get it going in general.

davidrohr · 2021-04-09T07:39:11Z

What do I have to run to test this? Restart from o2-sim? or from o2-sim-digitizer-workflow?

peressounko · 2021-04-09T08:12:22Z

Hi @davidrohr, you have to start from simulation as noise simulation changed there.

sevdokim · 2021-04-09T09:59:15Z

Hi @davidrohr, you have to start from simulation as noise simulation changed there.

Hello!

Exactly, one needs to start from the scratch. Not only noise but signal simulation changed (signal is scaled by factor 100 and
default calib coefs decreased by same value).

peressounko · 2021-04-09T10:04:34Z

Dear Sergey, I see you decide to add pedestals to Hits. Why do you need this? How do you plan to adding several hits in pileup simulation? You will have to subtract one (or several) redundant pedestals.
I think it is more practical to keep pure signal in Hits and to add pedestals and noise in Digitizer.
Best regards, Dmitri

sevdokim · 2021-04-09T10:10:12Z

Dear Sergey, I see you decide to add pedestals to Hits. Why do you need this? How do you plan to adding several hits in pileup simulation? You will have to subtract one (or several) redundant pedestals.
I think it is more practical to keep pure signal in Hits and to add pedestals and noise in Digitizer.
Best regards, Dmitri

Hello, Dmitri!

I didn't add pedestals to hits. I add pedestals when packing raw from digits. So the workflow if following: hits -> digits -> raw (add pedestals here) -> digits (subtract pedestals here) -> clusters.

sevdokim · 2021-04-09T10:15:51Z

Dear Sergey, I see you decide to add pedestals to Hits. Why do you need this? How do you plan to adding several hits in pileup simulation? You will have to subtract one (or several) redundant pedestals.
I think it is more practical to keep pure signal in Hits and to add pedestals and noise in Digitizer.
Best regards, Dmitri

Ahh, I see. I added pedestals to Detector.h during development and forgot to exclude them when realized that we don't need them. Can I remove it in the next my commits?

peressounko · 2021-04-09T10:56:19Z

Dear Sergey, It would be better to have operational CPV in o2 tags. I would prefer to fix this in current PR. Best regards, Dmitri

sevdokim · 2021-04-09T11:01:53Z

Dear Sergey, It would be better to have operational CPV in o2 tags. I would prefer to fix this in current PR. Best regards, Dmitri

This is operational, just a matter of 2 excess includes. However as you wish.
Regards,
Sergey

sevdokim · 2021-04-09T11:06:46Z

Dear Sergey, It would be better to have operational CPV in o2 tags. I would prefer to fix this in current PR. Best regards, Dmitri

I checked again and didn't find anything what are talking about. Please consider my previous message as wrong.
Would it be possible to point exact lines which questioned you?

peressounko · 2021-04-09T11:57:36Z

Dear Sergey, you are right, I confused Detector.cxx and Digitizer.cxx in review. There is no adding Pedestals in Detector as it should be. Sorry for the noise. Dmitri.

sevdokim · 2021-04-09T12:02:00Z

Dear Sergey, you are right, I confused Detector.cxx and Digitizer.cxx in review. There is no adding Pedestals in Detector as it should be. Sorry for the noise. Dmitri.

Same for me))

sevdokim · 2021-04-09T19:30:23Z

Dear @davidrohr, @ktf

I believe the fullCI is faulty and should be fixed. Please point my errors if I'm wrong. Otherwise please merge my PR.

Thank you, regards,
Sergey

shahor02 · 2021-04-09T19:57:17Z

@sevdokim The fullCI is indeed broken by me, the PR with fix is being tested, let's wait for it to be merged then we can merge your PR once the fullCI is green.

sevdokim · 2021-04-09T20:04:14Z

Hi @shahor02
thanks for the info. Will it run automatically on the PR? I see that now it's just in unsuccessful state, nothing queued or running.
And no buttons for me to run fullCI again.
PS: thanks for adding emacs backups into .gitignore!

davidrohr · 2021-04-09T20:25:35Z

For reference, I checked the performance and it is ~40x faster than before, very good and thx a lot! Should be squash-merged as soon as the full CI has rerun.

shahor02 · 2021-04-09T20:01:04Z

Detectors/CPV/simulation/src/RawWriter.cxx

+      LOG(INFO) << "[RawWriter] getting calibration object from ccdb";
+      o2::ccdb::CcdbApi ccdb;
+      std::map<std::string, std::string> metadata;
+      ccdb.init("http://ccdb-test.cern.ch:8080"); // or http://localhost:8080 for a local installation


could you please make the ccdb patch a settable parameter?

shahor02 · 2021-04-09T20:02:05Z

Detectors/CPV/workflow/src/RawToDigitConverterSpec.cxx

+      //TODO: configuring ccdb address from config file, readign proper calibration/BadMap and updateing if necessary
+      o2::ccdb::CcdbApi ccdb;
+      std::map<std::string, std::string> metadata;
+      ccdb.init("http://ccdb-test.cern.ch:8080"); // or http://localhost:8080 for a local installation


Dear @shahor02, can you advice a way to make it configurable? You mean to read it in init() function from framework::InitContext& ctx?
I ask because I can do something which no one will like due to my incomplete understanding.

@sevdokim, just define e.g. mCCDBUrl data member which can be set via workflow option (with proper default, e.g. "http://ccdb-test.cern.ch:8080"). For the reference, look on

AliceO2/Detectors/ZDC/workflow/src/ZDCDataReaderDPLSpec.cxx

Line 67 in b80c66b

framework::DataProcessorSpec getZDCDataReaderDPLSpec(const RawReaderZDC& rawReader, const std::string& ccdbURL, const bool verifyTrigger)

called from

AliceO2/Detectors/ZDC/workflow/src/o2-zdc-raw2digits.cxx

Lines 44 to 54 in b80c66b

auto ccdbURL = configcontext.options().get<std::string>("ccdb-url");

auto checkTrigger = true;

auto notCheckTrigger = configcontext.options().get<bool>("not-check-trigger");

if (notCheckTrigger) {

LOG(INFO) << "Not checking trigger condition during conversion";

checkTrigger = false;

}

o2::conf::ConfigurableParam::updateFromString(configcontext.options().get<std::string>("configKeyValues"));

WorkflowSpec specs;

specs.emplace_back(o2::zdc::getZDCDataReaderDPLSpec(o2::zdc::RawReaderZDC{dumpReader}, ccdbURL, checkTrigger));

.

For the

AliceO2/Detectors/CPV/simulation/src/RawWriter.cxx

Line 71 in 3e937b0

ccdb.init("http://ccdb-test.cern.ch:8080"); // or http://localhost:8080 for a local installation

it will be slightly different, since the digit2raw is not a workflow but simple executable. See e.g.

AliceO2/Detectors/ZDC/simulation/src/digi2raw.cxx

Line 60 in b80c66b

add_option("ccdb-url,c", bpo::value<std::string>()->default_value(""), "url of the ccdb repository");

for the usage of configurable ccdb, though ZDC sets its up in the digi2raw.cxx itself, you will need to propagate the option to RawWriter.cxx.

BTW, for the ccdb reading it is preferable to use the BasicCCDBManager (wrapper around the CcdbApi) since it supports caching.

shahor02 · 2021-04-09T20:28:38Z

Detectors/CPV/calib/include/CPVCalib/Pedestals.h

 #define CPV_PEDESTALS_H

 #include <array>
 #include "TObject.h"


If this is a calibration data format which should be stored in the ccdb, please consider moving it to DataFormats/Detectorts/CPV (can be other PR, I guess there are other such classes).

@shahor02 actually there is plenty of work: I need to adjust calibration procedure as well. I think configuring ccdb path should naturally be included in that PR (which is not ready yet). I hope it's OK for you.

@sevdokim Sure, but in the DPL workflows keep it simple: eventually the DPL framework will take care of CCDB related stuff.

@shahor02 to be honest, I didn't get you last sentence. Can you please detalize it?
Particularly, what is the reason to move calibration objects headers to DataFormats/Detectorts/CPV? Will it help CCDB framework somehow to take care of them? What is CCDB related stuff? In our code we connect to CCDB and exctract
calibration objects from there. Maybe there is a global instance of CCDB which can be used and we don't have to create a new one?
PS: sorry for this ignorance. I want to improve my understanding in order to produce effective code and safe your time for reviews in the future.

@sevdokim We try to have all data (including the calibration) format classes (i.e. those classes which ends up in the DPL messages and/or stored in the files) in DataFormats directories, with no dependence on Detectors sim/rec/calib processing classes. Mostly in order to minimize the risk of circular dependencies.

Concerning the "CCDB stuff": eventually, the detector code should not deal with CCDB server directly, instead it will declare in the DPL device specifications what objects it will need (in the same way as it currently subscribes to data) and the framework will take care of shipping the objects matching to the timestamp of the TF being processed.
This is not available at the moment so we have to configure/query the ccdb server. Note that this will work for DPL workflows only. For simple executables, like digi2raw, we will always deal with CCDB directly.

shahor02 · 2021-04-09T20:32:19Z

In principle, once dev is updated by the fix, it should rerun automatically, if it will not, will ask Timo to restart.

davidrohr · 2021-04-11T13:49:20Z

This has passed the full CI, no idea why it is marked red. Merging since it is a significant improvement. Further work can happen in a separate PR.

* Fixed CPV simulation workflow * applied clang-format * fix for ~statement should be inside braces~ mumbling Co-authored-by: sevdokim <sergey.evdokimov@cern.ch>

sevdokim added 2 commits April 8, 2021 14:39

Fixed CPV simulation workflow

8e4543a

applied clang-format

5acd63c

sevdokim requested review from kharlov and peressounko as code owners April 8, 2021 13:20

kharlov previously approved these changes Apr 8, 2021

View reviewed changes

fix for ~statement should be inside braces~ mumbling

3e937b0

sevdokim dismissed kharlov’s stale review via 3e937b0 April 9, 2021 06:47

sevdokim requested a review from kharlov April 9, 2021 06:49

kharlov approved these changes Apr 9, 2021

View reviewed changes

shahor02 reviewed Apr 9, 2021

View reviewed changes

davidrohr merged commit e059886 into AliceO2Group:dev Apr 11, 2021

sevdokim deleted the fix-cpv-simulation branch April 12, 2021 15:12

	auto ccdbURL = configcontext.options().get<std::string>("ccdb-url");
	auto checkTrigger = true;
	auto notCheckTrigger = configcontext.options().get<bool>("not-check-trigger");
	if (notCheckTrigger) {
	LOG(INFO) << "Not checking trigger condition during conversion";
	checkTrigger = false;
	}

	o2::conf::ConfigurableParam::updateFromString(configcontext.options().get<std::string>("configKeyValues"));
	WorkflowSpec specs;
	specs.emplace_back(o2::zdc::getZDCDataReaderDPLSpec(o2::zdc::RawReaderZDC{dumpReader}, ccdbURL, checkTrigger));

CPV fix simulation workflow #5887

CPV fix simulation workflow #5887

Uh oh!

Conversation

sevdokim commented Apr 8, 2021

Uh oh!

davidrohr commented Apr 8, 2021

Uh oh!

sevdokim commented Apr 9, 2021

sw/BUILD/o2checkcode-latest/log

Uh oh!

davidrohr commented Apr 9, 2021

Uh oh!

davidrohr commented Apr 9, 2021

Uh oh!

peressounko commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

peressounko commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

peressounko commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

peressounko commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

shahor02 commented Apr 9, 2021

Uh oh!

sevdokim commented Apr 9, 2021

Uh oh!

davidrohr commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sevdokim Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shahor02 commented Apr 9, 2021

Uh oh!

davidrohr commented Apr 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

sevdokim Apr 10, 2021 •

edited

Loading