Skip to content

fix in tof calib data structure#7641

Merged
shahor02 merged 2 commits intoAliceO2Group:devfrom
noferini:dev
Nov 18, 2021
Merged

fix in tof calib data structure#7641
shahor02 merged 2 commits intoAliceO2Group:devfrom
noferini:dev

Conversation

@noferini
Copy link
Collaborator

No description provided.

@noferini noferini requested a review from shahor02 November 17, 2021 13:00
@noferini
Copy link
Collaborator Author

@chiarazampolli

vectors per sector splitted to solve a problem in ROOT streamer.

Now all calibrations (for all sectors) are loaded. Tested in full workload with quick calibrations.

I am going to reproduce full calibrations with this structure

@shahor02
Copy link
Collaborator

Hi @noferini

The drawback of such a workaround is that the object which could be messegeable (POD) is not anymore since it has pointers.
I don't thing root parallelizes the reading. You could do simply

std::array<int, NCHANNELS> mChannelStart;
int getStartIndexForChannel(int sector, int channel) const { return mChannelStart[sector*NCHANNELXSECTOR + channel]; }

BTW, what is the reason to use a vector for mTimeSlewingSec... rather than fixed size array?

@noferini
Copy link
Collaborator Author

Hi @noferini

The drawback of such a workaround is that the object which could be messegeable (POD) is not anymore since it has pointers. I don't thing root parallelizes the reading. You could do simply

std::array<int, NCHANNELS> mChannelStart;
int getStartIndexForChannel(int sector, int channel) const { return mChannelStart[sector*NCHANNELXSECTOR + channel]; }

I want to use parallelization at the level of calibration fitters (time slewing per sector). It is not related to ROOT.

BTW, what is the reason to use a vector for mTimeSlewingSec... rather than fixed size array?

We left free the number of ToT points per channels depending on the statistics we will accumulate (more statistics -> more points) accordingly to what done in Run-2

@noferini
Copy link
Collaborator Author

@shahor02
Are you sure it is not messageable?
Actually, I sent it to the populator to fill ccdb and I was able to read it back (but for writing pointers are not used, I'll check if it complain in case I try to use them after message passing).

@shahor02
Copy link
Collaborator

I want to use parallelization at the level of calibration fitters (time slewing per sector). It is not related to ROOT.
But then how does this depend on the way you segment this container.

We left free the number of ToT points per channels depending on the statistics we will accumulate (more statistics -> more points) accordingly to what done in Run-2

I don't see in the code how it is used. How it should be related to channel?

Are you sure it is not messageable?

Yes. Messageably means that it can be passed from device to device as binary blob, e.g. via shared memory. Should not be confused by (root) serializable object, there passing the object involves a data copy + streaming.

@noferini
Copy link
Collaborator Author

I want to use parallelization at the level of calibration fitters (time slewing per sector). It is not related to ROOT.
But then how does this depend on the way you segment this container.

We left free the number of ToT points per channels depending on the statistics we will accumulate (more statistics -> more points) accordingly to what done in Run-2

I don't see in the code how it is used. How it should be related to channel?

mChannelStart[sector][channel] points where "channel" calibration starts in the vector
mChannelStart[sector][channel+1] points where "channel" calibration stops in the vector
vector is a pair of ToT and Correction

Are you sure it is not messageable?

Yes. Messageably means that it can be passed from device to device as binary blob, e.g. via shared memory. Should not be confused by (root) serializable object, there passing the object involves a data copy + streaming.

I can remove the pointer leaving all the rest as it is.

@shahor02
Copy link
Collaborator

I can remove the pointer leaving all the rest as it is.

There is no point in doing this if you keep the vector as a data member: it is also not messageable since it contains a pointer.

@noferini
Copy link
Collaborator Author

I can remove the pointer leaving all the rest as it is.

There is no point in doing this if you keep the vector as a data member: it is also not messageable since it contains a pointer.

I will not keep any vector (just std::array and std::vector). I'm going to remove the vector of pointers at all

@shahor02
Copy link
Collaborator

By vector I mean std::vector, as all dynamically expandable containers it has a pointer inside.

@noferini
Copy link
Collaborator Author

By vector I mean std::vector, as all dynamically expandable containers it has a pointer inside.

Ok, but this cannot be avoid without changing completely the calibration scheme.
Again, it is not clear why we have a problem here to use vector which are used in all our data structures.

@noferini
Copy link
Collaborator Author

noferini commented Nov 17, 2021

Again, it is not clear why we have a problem here to use vector which are used in all our data structures.

Forgot this comment (I am tired)

@shahor02
Copy link
Collaborator

Hi @noferini
It is not problem per se, it is just inefficient for the classes which are messaged. In principle, we do avoid when possible using dynamic containers in the DataFormats classes, moreover such a large one as this one. Anyways, if changing this is complicated and it is messaged only from the calibrator to the CCDBPopulator, we can live with that (unless we see that this creates a real problem)

@noferini
Copy link
Collaborator Author

Hi @noferini It is not problem per se, it is just inefficient for the classes which are messaged. In principle, we do avoid when possible using dynamic containers in the DataFormats classes, moreover such a large one as this one. Anyways, if changing this is complicated and it is messaged only from the calibrator to the CCDBPopulator, we can live with that (unless we see that this creates a real problem)

At some point it could be also passed as message from the ccdb-service if required as input Lifetime::Condition.
This is what I am thinking to...
In any case I would suggest to take this PR as it is which is in any case better than the previous situation.
I'll think if there is a different way to do the same job in the next day...

@shahor02
Copy link
Collaborator

failures unrealated.

@shahor02 shahor02 merged commit 2579f68 into AliceO2Group:dev Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants