TRD and TPC constants in subsidiary namespace

[martenole]

Hi @jolopezl @bazinski and @tdietel

I was going through the TRD code in O2 and noticed that some fixed constants of the TRD (Nlayers, Nstacks, ...) are defined quite often in different header files. Shall we collect these constants and add them to a separate header file? The TPC is doing the same and it would make our code more readable I think.
I could replace all obsolete definitions, but I don't want to mess around with the same files that you are working on. Please let me know what you think.

Cheers,
Ole

[bazinski]

Hi @martenole
Cool, I was just looking through this in aliroot earlier for pr4121, but why a class why not simply static constexpr in the namespace ? o2::trd::NSectors ?

You want to pull all the stuff from FeeParam, Geometry and TRDSimParam ? It was discussed at one point, I dont remember why it stalled.

[martenole]

Hi Sean,
concerning the class: This is simply how it was done for the TPC. We can also put it in a namespace, but then I would suggest not to use o2::trd, but o2::trd::constants, to avoid unwanted duplications. But then the amount of writing in the code is again the same as with a class. I don't really have an opinion on this.

Yes, I would take anything from the classes you mention which is used in various places. Definitely the geometrical constants of FeeParam, but not for example variables like kNPadsInPadResponse of the TRDSimParam, since this is specific to the simulation. And all the detailed definitions in TRDGeometryBase I would also leave untouched, because I would say this is the natural place for them.

[bazinski]

Hi Ole
Cool class it is in then I suppose. Ja I was suggesting it to conserve typing ;-)
So then its all of :

• mgk.... in FeeParam ?

I see there is a mgkNrowC1, MAXROWS, ROWMAXC1, maybe more than just my opinion is needed here.
I assume you want to use this right now, so maybe go with what you have and add stuff later?

[martenole]

This is not urgent, I want to wait for Jorge to see if he sees anything that speaks against this.

Exactly, all mgk... but the LHC frequency which is already defined in LHCConstants.h ;)

For the number of rows per chamber we just need to choose a naming convention. Maybe NROWSC0 (12 rows in stack 2) and NROWSC1 (16 rows everywhere else) is best.

[jolopezl]

Hello @martenole, this will improve the code a lot. It happens to me that many times I am using the same constant from several places, for example, with kNdet, which is defined in at least two different places.
My personal preference would be to put all constants that will never change in a namespace, namely, o2::trd::constants, but I guess it is better to keep consistency across different detector systems, then having a class is ok, too. I am not sure about what is the best C++ practice in this matter.

[martenole]

Hi @davidrohr
I was wondering if there is a difference between putting constants in a class, like you did it for the TPC constants and putting them in a namespace, as it is done for example for the LHC parameters.
Since one does not create an instance of the class, there should be no difference, right? Is there a best practice?
Cheers,
Ole

[davidrohr]

I think technically there is not really a difference and so far some detectors use the one and others use the other way. I think at some point there was a discussion to limit the nested level of namespaces, so if in doubt I would use a class, but it doesn't really matter.

[bazinski]

Looks cool.

[tdietel]

I think technically there is not really a difference and so far some detectors use the one and others use the other way. I think at some point there was a discussion to limit the nested level of namespaces, so if in doubt I would use a class, but it doesn't really matter.

I am wondering about this and the wish to reduce keyboard wear: I guess with a namespace one could do using o2::trd::constants::NSECTOR, and then use NSECTOR. Is something similar possible with a class?

But my typing might have already created more keyboard wear than that ;-) I'm happy either way.

[tdietel]

See my comment, otherwise I'm happy.

[jolopezl]

Looks great!

[martenole]

After talking to @davidrohr the variables are now put in a namespace instead of a class. Will do the same for the TPC constants now in an additional commit.
Afterwards this can be merged

[jolopezl]

After talking to @davidrohr the variables are now put in a namespace instead of a class. Will do the same for the TPC constants now in an additional commit.
Afterwards this can be merged

That's great! Just out of curiosity, did you discuss some technicalities for using a namespace rather than a class?

[martenole]

That's great! Just out of curiosity, did you discuss some technicalities for using a namespace rather than a class?

Not really.. We think that technically there is probably no difference, 3 levels of namespaces should be fine and Tom had a good point with less typing when one puts a using namespace o2::trd::constants. So it's just convenience ;)

[davidrohr]

This looks better to me now, and it is less typing than before.
Will you squash the cleanup commit into the TRD commit, or do you want me to squash anything when merging? I believe the errors are unrelated.

[ktf]

Yes. Errors are unrelated. @knopers8 can you look at them? These are with 1.0.2.

[martenole]

Hi @davidrohr
Yes, I will squash them. But the errors actually are related I am afraid. In QualityControl in https://github.com/AliceO2Group/QualityControl/blob/master/Modules/TPC/src/Clusters.cxx DataFormatsTPC/Constants.h is used.. So we would need a patch also for QC, right?

[davidrohr]

@martenole : Nicely spotted, QC was failing all the time for unrelated reasons, so I ignored it...
Yes, we need a PR also for QC. Could you prepare that?
Then we have to manually quickly merge this one, merge the QC one, and bump QC @ktf @Barthelemy @knopers8

[knopers8]

I would advocate doing it in gentle steps without breaking the builds, but if it is not possible here, then sure, I can help you.
Please make sure that there are also no warnings in O2 and QC during compilation. QC treats all warnings as errors, O2 probably too, if i remember correctly.

[davidrohr]

We could do everything in QC with an #ifdef first, support both versions, then switch in O2, then remove the stuff in QC. But since everything requires also an alidist bump, that will be quite annoying, so I'd rather do it quickly in one go.

[knopers8]

OK, let me know when you are ready

[knopers8]

@martenole : Nicely spotted, QC was failing all the time for unrelated reasons, so I ignored it...

BTW, QC was failing yesterday because we were in progress of doing the same exact thing, but for EMCal-related code. So you can see the amount of chaos it introduces.

[davidrohr]

@martenole : I think we can avoid it this way:

namespace a {
    int x;
}
namespace A {
    using namespace a;
}
int main(int argc, char** argv) {
    int foo = a::x;
    int bar = A::x;
}

We can just provide the old Constants as an alias for a short time until we update QC?

[martenole]

Hi @davidrohr
this is a very good idea! I will push this as soon as my build is ready after yet another rebase on dev which I did since my QC build was failing for something which seemed unrelated to these changes... Maybe I ask Johanna for a new computer at some point :)

[martenole]

Hi @shahor02 and @wiechula
I think your approval is required to merge this PR. It simply changes the o2::tpc::Constants class to a namespace o2::tpc::constants. As suggested by David an alias to the old Constants class ensures compatibility to QualityControl. This alias will be removed once PR459 is merged and QC is bumped to include that change.
Is this okay with you?
Cheers,
Ole

[davidrohr]

@martenole : Thx for the work, can you open a corresponding PR for QC as well?

[martenole]

@davidrohr Thanks for merging, PR for QC is here: AliceO2Group/QualityControl#459

[knopers8]

Thanks for finding a way to make it smooth!