Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Porting latest ITSU geometry from AliRoot v2 to O2 #383

Merged
merged 4 commits into from
Jun 14, 2017
Merged

Porting latest ITSU geometry from AliRoot v2 to O2 #383

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
409859f
Porting latest ITSU geometry from AliRoot v2 to O2
May 18, 2017
20e2e28
First round of code review implementation
May 18, 2017
e64cf21
Second round of fixes for ITSU IB porting
Jun 13, 2017
40c2677
Adding Copyright note to new files
Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Detectors/ITSMFT/ITS/simulation/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ O2_SETUP(NAME ${MODULE_NAME})
set(SRCS
src/V11Geometry.cxx
src/V1Layer.cxx
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If V1Layer is not used anymore, consider removing the file completely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

src/V3Layer.cxx
src/Detector.cxx
src/GeometryHandler.cxx
src/Digitizer.cxx
Expand All @@ -22,6 +23,7 @@ set(HEADERS
include/${MODULE_NAME}/DigitizerTask.h
include/${MODULE_NAME}/GeometryHandler.h
include/${MODULE_NAME}/V1Layer.h
include/${MODULE_NAME}/V3Layer.h
include/${MODULE_NAME}/V11Geometry.h
)

Expand Down
26 changes: 14 additions & 12 deletions Detectors/ITSMFT/ITS/simulation/include/ITSSimulation/Detector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for my lack of experience: could you please suggest me what I should improve here ?

Copy link
Collaborator

@sawenzel sawenzel May 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ops, sorry, I thought something more C++ in-deep :)
I took out all obsolete/out of sync comments.


namespace o2 {
namespace ITS {

class V1Layer;
class V3Layer;

class Detector : public o2::Base::Detector
{
Expand All @@ -40,14 +40,16 @@ class Detector : public o2::Base::Detector
enum Model
{
kIBModelDummy = 0,
kIBModel0 = 1,
kIBModel1 = 2,
kIBModel21 = 3,
kIBModel22 = 4,
kIBModel3 = 5,
kOBModelDummy = 6,
kOBModel0 = 7,
kOBModel1 = 8
kIBModel0 = 1,
kIBModel1 = 2,
kIBModel21 = 3,
kIBModel22 = 4,
kIBModel3 = 5,
kIBModel4 = 10,
kOBModelDummy = 6,
kOBModel0 = 7,
kOBModel1 = 8,
kOBModel2 = 9
};

/// Name : Detector Name
Expand Down Expand Up @@ -277,7 +279,7 @@ class Detector : public o2::Base::Detector
Double_t *mLayerZLength; //! Vector of layer length along Z
Int_t *mStavePerLayer; //! Vector of number of staves per layer
Int_t *mUnitPerStave; //! Vector of number of "units" per stave
Double_t *mStaveThickness; //! Vector of stave thicknesses
Double_t *mChipThickness; //! Vector of chip thicknesses
Double_t *mStaveWidth; //! Vector of stave width (only used for turbo)
Double_t *mStaveTilt; //! Vector of stave tilt (only used for turbo)
Double_t *mDetectorThickness; //! Vector of detector thicknesses
Expand Down Expand Up @@ -306,7 +308,7 @@ class Detector : public o2::Base::Detector
GeometryHandler *mGeometryHandler;
MisalignmentParameter *mMisalignmentParameter;

V1Layer **mGeometry; //! Geometry
V3Layer **mGeometry; //! Geometry
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be feasible to use stl containers here? Is there any strong reason to stay with **?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for my lack of experience: could you please suggest me what I should improve here ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mario6829 Hi Mario. I would say, yes, let's do these STL-related changes in a separate, self-consistent PR.

Model mStaveModelInnerBarrel; //! The stave model for the Inner Barrel
Model mStaveModelOuterBarrel; //! The stave model for the Outer Barrel

Expand Down
Loading