Porting latest ITSU geometry from AliRoot v2 to O2 #383
Conversation
|
Error while checking build/o2checkcode/o2-daq for cdd92434f60c2b6dbf28c33393e41ed18b92d2f0: Full log here. |
|
@iouribelikov : Can you take a look at this? |
| @@ -306,7 +308,7 @@ class Detector : public o2::Base::Detector | |||
| GeometryHandler *mGeometryHandler; | |||
| MisalignmentParameter *mMisalignmentParameter; | |||
|
|
|||
| V1Layer **mGeometry; //! Geometry | |||
| V3Layer **mGeometry; //! Geometry | |||
There was a problem hiding this comment.
would it be feasible to use stl containers here? Is there any strong reason to stay with **?
There was a problem hiding this comment.
I apologize for my lack of experience: could you please suggest me what I should improve here ?
There was a problem hiding this comment.
Probably you want to express something like array of V3Layers. So, I am wondering if we could use something like std::vector<V3Layer*> or std::vector<unique_ptr<V3Layer>> or similar. We can possibly address this in a separate improvement PR.
There was a problem hiding this comment.
@sawenzel I agree with you, Sandro...
In fact, if we look at the data member declarations inside Detector.h, we will find about 15 similar cases more: raw pointers to some arrays allocated later with "new". They would call for a similar improvement as well. Moreover, all those array are actually of a known size, which is 7 (the number of layers, frozen now). Potentially, we could use also this fact to simplify our geometry code even more.
Doing it in a clean way may take quite some additional time. The changes would be important from the code maintenance point of view, and at the end, they would still result in the same "geometry tree".
So, I think, we should do it in a separate (maybe not even immediately next) PR.
There was a problem hiding this comment.
So if I understand correctly, this can be left as it is for the time being, and then I'll produce an updated version of the code using the proper STL coding and submit a second pull request: am I right ?
There was a problem hiding this comment.
@mario6829 Hi Mario. I would say, yes, let's do these STL-related changes in a separate, self-consistent PR.
| @@ -26,12 +26,12 @@ namespace o2 { namespace ITSMFT { class Point; }} // lines 22-22 | |||
| namespace o2 { namespace ITS { class GeometryHandler; }} | |||
| namespace o2 { namespace ITS { class MisalignmentParameter; }} | |||
| namespace o2 { namespace ITS { class GeometryTGeo; }} | |||
| namespace o2 { namespace ITS { class V1Layer; }} // lines 23-23 | |||
| namespace o2 { namespace ITS { class V3Layer; }} // lines 23-23 | |||
There was a problem hiding this comment.
what does the comment actually mean? If it is where in the code the class is used, it is quickly going to be out of sync. And it is probably already
There was a problem hiding this comment.
I apologize for my lack of experience: could you please suggest me what I should improve here ?
There was a problem hiding this comment.
I think @matthiasrichter is referring to the comments // lines 23-23. Indeed they don't carry much information and the line numbers are likely to be out of sync. So I would just remove them.
There was a problem hiding this comment.
ops, sorry, I thought something more C++ in-deep :)
I took out all obsolete/out of sync comments.
| if (mStaveThickness[j] != 0) { | ||
| mGeometry[j]->setStaveThick(mStaveThickness[j]); | ||
| if (mChipThickness[j] != 0) { | ||
| mGeometry[j]->setChipThick(mChipThickness[j]); |
There was a problem hiding this comment.
Is the V1Layer class supposed to work with this as well? Then the function SetStaveThickness must be renamed.
There was a problem hiding this comment.
The V1Layer class is obsolete and will never be used. Anyway if you think it is better, I can re-implement the deleted member and its Setter/Getter, but they won't be used anywhere.
There was a problem hiding this comment.
probably this is not necessary.
There was a problem hiding this comment.
Indeed, then it is probably not necessary, I would even go further and remove the v1 class from the repository
There was a problem hiding this comment.
Discussing this in the PWG1&2 it turned out to leave the V1 class as a reference at least for some time. Moreover, as Yura pointed out, it could be used again in the future to study a sort of "super-upgraded" version of the ITS. I would leave it for a while in the repository if it is not a problem.
| static const Double_t sOBSFBotBeamAngle; ///< OB SF bottom beam angle | ||
| static const Double_t sOBSFrameBeamSidePhi; ///< OB SF side beam angle | ||
|
|
||
| ClassDef(V3Layer, 0) // ITS v3 geometry |
There was a problem hiding this comment.
please use ClassDefOverride here (there should be a compiler warning about that)
There was a problem hiding this comment.
Indeed. We need to use ClassDefOverride consistently from now on.
There was a problem hiding this comment.
I changed ClassDef with ClassDefOverride, please let me know if it is ok.
| } | ||
| } | ||
|
|
||
| V3Layer::V3Layer(Int_t lay, Bool_t turbo, Int_t debug) |
There was a problem hiding this comment.
consider reordering the parameters like
V3Layer::V3Layer(Int_t debug, Bool_t turbo, Int_t lay)that leaves the debug parameter at the same position, default values can be defined and the constructors can be combined.
There was a problem hiding this comment.
I followed your suggestion and I got rid of all constructors but the most complete one, and I added default values for its parameters in its definition in V3Layer.h file. Please let me know if it is ok.
| } | ||
|
|
||
| V3Layer::V3Layer(const V3Layer &s) | ||
| : V11Geometry(s.getDebug()), |
There was a problem hiding this comment.
the compiler can do a good job here, if nothing special is needed it's better to define the copy ctor and assignment operator in the header file like
V3Layer(const V3Layer &) = default;
V3Layer & operator=(const V3Layer &) = default;There was a problem hiding this comment.
I got rid of my implementation of copy and assignment constructors, and used "default" in V3Layer.h, please let me know if it is ok.
|
Many thanks for your comments. Please see my replies to your request. |
| @@ -0,0 +1,449 @@ | |||
| /// \file AliITSUv1Layer.cxx | |||
| /// \brief Definition of the AliITSUv1Layer class | |||
There was a problem hiding this comment.
the name is probably out of sync (should it be v3 instead of v1)?
There was a problem hiding this comment.
yup, right. I fixed it putting the proper comment
| @@ -5,6 +5,7 @@ O2_SETUP(NAME ${MODULE_NAME}) | |||
| set(SRCS | |||
| src/V11Geometry.cxx | |||
| src/V1Layer.cxx | |||
There was a problem hiding this comment.
If V1Layer is not used anymore, consider removing the file completely.
There was a problem hiding this comment.
@sawenzel There was kind of an additional consideration (Ruben might know it better)...
At some point, we may be asked to do simulations for so called "super-upgrade". Because they are much less detailed, the old geometry classes (like V1Layer) are more easy to adapt for the super-upgrade purposes. So, I would keep the old stuff in the repository.
However, true, we my consider removing the references to these classes from the CMakeLists, LinkDefs, etc. For the moment at least.
There was a problem hiding this comment.
@mario6829 Sorry, Mario. When I try to run the new geometry inside the standard test (which uses run_sim_its_ALP3.C macro), I get a FATAL complaining about "Stave model 4 obsolete and no longer supported".
Should not something trivial be fixed inside the "central" macro/run_sim_its_ALP3.C ?
|
Error while checking build/o2checkcode/o2-daq for 20e2e28: Full log here. |
|
Hello, I apologize for the delay, for more than a week I wasn't able to compile. I fixed what you required, please see my replies to your comments for details. |
|
Hi Yura, I fixed the run_sim_its_ALP3.C macro so as to use the latest geometry version, should be ok now. |
|
Error while checking build/o2checkcode/o2-daq for e64cf21: Full log here. |
|
I added the copyright note to the new files as required by failing test (though all other already existing files seem not to contain it) |
|
Hello everybody, Shall we now merge this pull request ? Cheers, |
|
@iouribelikov : If you are fine with it ... we can proceed. Are you going to keep track of the idea of code modernization somewhere? Please still make a tick in your review part (you need to approve). |
|
@sawenzel Approved ! |
|
Thanks for these improvements. Merging. |
|
Yes we can do that on github. Would you open it?
2017-06-14 11:05 GMT+02:00 iouribelikov <notifications@github.com>:
… @sawenzel <https://github.com/sawenzel> Approved !
Now concerning the code modernization, should we open a "github issue" ?
Or, what would the best way to track it ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#383 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADX3cfae4f2c4klx2HVbKo1kkbqNibeSks5sD6JOgaJpZM4NfNn5>
.
--
Dr. Sandro Wenzel
CERN (EP / AIP / SDS)
sandro.wenzel@cern.ch
|
|
@sawenzel @mario6829 Done: #414 |
* Clarify doc for reading raw data * Update QuickStart.md
- Add event selection - Add Y dependent efficiency - Add TOF efficiency
New V3Layer geometry file created which contains the latest ITSU Inner Barrel description as ported from current AliRoot v2 version.