Project

General

Profile

Feature #1216

Code Review: Global Recon data structure

Added by Rogers, Chris over 8 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Taylor, Ian
Category:
Detector Integration
Target version:
Start date:
01 February 2013
Due date:
% Done:

100%

Estimated time:
Workflow:
Closed

Description

Outcome of code review for global reconstruction

#1

Updated by Rogers, Chris over 8 years ago

From Peter Lane:

Since you're seeking a code review of the data structure code, I'd like to add my $0.02 based on what I see in your dev branch (hopefully that's the latest version).

  1. We need to merge the enums in GlobalReconEnums.hh with those in Detector.hh and Particle.hh. Selfishly I'd like to see the ones I've been using be adopted as much as possible from it's current form to avoid too much recoding, but I'm open to suggestions on changes you see necessary. Nit picking a bit as well, I don't like the feel of having a header for just the enums in DataStructure. It doesn't strike me as a very C++ thing to do.
  2. I'm not sure I like the idea of having the classes in the MAUS::recon::global and then typedefing them into the MAUS namespace and tacking on "Global" on the front. I'm inclined to say that they should all just be Global* classes in the MAUS namespace. They aren't really serving as functional components to the internals of the reconstruction. Like all the other classes in DataStructure they are intended for data passing. So in some sense they are polluting the MAUS::recon::global namespace.
  3. It looks like GlobalTrackPoint should be a subclass of GlobalSpacePoint.
  4. Nit pick: if the class name is SpacePoint then an instance variable should be space_point, not spacepoint. Same for TrackPoint.
  5. I like author comments in source code files. It lets me know who to talk to if I need to ask a question about something.
  6. I don't like the *FromInt function names in GlobalPrimaryChain. It doesn't tell you what the int you are using represents. Better: *At, *AtIndex, etc...
  7. I agree with the comment for CleanUpTracks in GlobalPrimaryChain about never removing tracks. Again, these are data passing classes. You shouldn't be using them internally in algorithms where you might need to move tracks around. Setup the instance once how you want it transmitted and leave it alone. Recommend deleting this function as well as the Add* and Delete* functions. The std::vector class has sufficient capabilities along these lines.

And for amusing attempts to push conformance to the Google coding standards we're supposed to adhere to:

  1. Member variables end, not begin, with an underscore. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names)
  2. Member variable getters are just the name of the instance variable without the underscore. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names)

Peter

#2

Updated by Rogers, Chris over 8 years ago

From Chris Rogers (actions on Ian unless stated)

Nb: reviewed revision 706 of bzr+ssh://bazaar.launchpad.net/~i-taylor/maus/rc/

PointerTRef/etc
  1. Worth commenting that TObject inheritance should be confined to DataStructure
  2. Can probably use TObject inheritance instead of template <class ChildType>
  3. action on ROGERS: Remove ObjectProcessor::RegisterPointerReference - this is superceded by RegisterTRef
  4. A comment on TRefArray "gotchas" would be useful - given that you've learnt from experience
DataStructure
  1. Remove typedefs - instead use MAUS::DataStructure::Global namespace, and reflect in the directory structure
  2. Add a pointer from PrimaryChain going back down the global recon tree of PrimaryChains - to allow more sophisticated back tracing than the mapper_name string
  3. Instances of Squeak::mout(Squeak::error) should probably be an exception
  4. Careful about memory ownership. If a class owns memory, should always have in mutator something like
    if (_data != NULL) {
        delete _data;
    }
    _data = new_data;
    

    Functions which take a pointer as an input should always be clear about who owns the memory before/after the function call - and this should be commented in the header doxygen.
  5. Add global datastructure to the doc/doc_src/spill_structure.dot
  6. Update data_structure.tex subsection about pointers and memory ownership
  7. Check test coverage (change the maus_lcov environment variable before running scons -c; scons; test/unit_tests.bash
#3

Updated by Taylor, Ian over 8 years ago

  • Workflow changed from New Issue to In Development

With respect to PointerTRef item #2:

The vast majority of the code can use a TObject*. The only stumbling block is in ReferenceResolverCppToJson, where a set of Static PointerValueTables has been created. The tables are populated with entries at one stage of the code, and then searched for matchign pointers later. However, as this code is templated, the table that is populated is e.g.

TypedPointerValueTable<MAUS::DataStructure::Global::SpacePoint*>

but the table that is searched is

TypedPointerValueTable<TObject*>

We could ensure that TypedPointerValueTable<TObject*> was populated in parallel, but the only way I can think to do this is a check (at run time) of inheritance, using casting. This is forbidden by the Google style guide, and does lead to the possibility of future confusion.

I am happy to go either way at this point. If someone knows of a way to work this that fits into our rules, then that would be best. Otherwise, I suggest that we leave the templates as they currently are...

Ian

#4

Updated by Taylor, Ian over 8 years ago

With a little more searching, this seems to conform to our rules, although it is also a little bit of Black Magic:

http://stackoverflow.com/questions/2633787/compile-time-type-determination-in-c

Any complaints before I implement it?

#5

Updated by Rogers, Chris over 8 years ago

Just to be clear - style guide is supposed to be a set of reasonable things one can do to make the code better. Don't make the code worse to obey style guide.

#6

Updated by Taylor, Ian over 8 years ago

Request from Victoria Blackmore @ CM35:

Add a data element to PrimaryChain, which tracks any potential issues or comments for analysers to be aware of in an event.

Would propose a std::map<"std::string", "std::string">, where first is an error/instance code, and second is details of the error/instance: e.g.

"OutsideTracker","Track left Tracker geometry region @ X:120, Y:-100, Z:-400"

#7

Updated by Taylor, Ian over 8 years ago

  • Workflow changed from In Development to Awaiting Merge

The code has been added to the merge branch. Almost all the comments above have been addressed, with the exceptions of:

  1. Combining the ENums. I agree that a combined set should be prepared, and I'm happy to use the Peter's style, however I dispute that a header is an inappropriate location and I think that the ENums should definitely end up inside the DataStructure namespace. As this will require changes to Peter's code, we should come to an agreement before we edit anything.
  2. By 'subclass', I'm assuming Peter means that TrackPoints could inherit from SpacePoints. Normally I would agree, however there is a small possibility of the code changing so that new elements of a specialised SpacePoint don't exist for TrackPoints. Safest option long term is to keep them seperate.

Ian

#8

Updated by Lane, Peter over 8 years ago

1. The enums are not strictly related to data passing, so I'd suggest just typedefing the enums from the recon code if you want a DS branded copy.

2. In that case the OOP thing to do would be to create a third class (GlobalPointBase or something) with common elements and have them both inherit it.

#9

Updated by Taylor, Ian over 8 years ago

  1. Discussed this with Chris Rogers to confirm the MAUS design strategy. DataStructure code shoudl not depend on other elements, and should form the base of MAUS. As the enums are being written into the data, it is important that they be defined inside the data structure (a future situation can be imagined, where data from MAUS 1.4.3 is analysed with MAUS 1.4.5, because we have confirmed that the DataStructure code did not change between the two versions, but the analysis tools did; this only works if DataStructure is self-contained). However, it should be trivial for me to import the current enums, add any elements that are missing and then put a typedef into the current GlobalRecon enum locations.
  2. We could, but we don't have to, and I don't see a compelling argument to do so.
#10

Updated by Lane, Peter over 8 years ago

1. Point taken.
2. No, of course you don't have to. But one of the main ideas of using an OO language is to take advantage of code reuse to avoid inconsistencies resulting from having to keep multiple pieces of code in sync. If the similar code is similar because it acts on the same subset of data then there ought to be a common definition in the data structure for that subset. Perhaps we've done our jobs perfectly and the DS subset will never change, but who knows. The whole point is that you don't have to worry about it because you've guaranteed that they can't go out of sync by design. TrackPoint and SpacePoint obviously qualify as two objects that contain the same subset of data. Anyway, that's my attempt at a compelling argument. Take it or leave it.

#11

Updated by Taylor, Ian over 8 years ago

1. And... you can't typedef enums that way. How very annoying! Going for the minimally painful option of changing Peter's code to point to DataStructure::Global for his enums...

2. Okay, I accept the argument.

#12

Updated by Lane, Peter over 8 years ago

1. Could you elaborate. I wrote a simple test program that typedefed an enum as a class member and it worked fine.

#13

Updated by Taylor, Ian over 8 years ago

Sorry, was rushing out the door to the field mitigation meeting, shouldn't have written such a short comment.

You can indeed typedef the enum, i.e. the following works and compiles:

typedef MAUS::DataStructure::Global::DetectorPoint ID;

However, this only typedefs the enum, it does not provide the values. This means that you get a failure from: Detector::kUndefined, with an error message claiming (rightly) that kUndefined is not a member of MAUS::recon::global::Detector.

I can get around this by adding:

using namespace MAUS::DataStructure::Global;

to every file and changing Detector::kUndefined to kUndefined, but seeing as that is a stupid thing to do, I'm not going to.

I'm about to call it a day, so if you have a snazy solution, then I'll implement it in the morning. Otherwise, I'll work through the code and make the necessary changes.

#14

Updated by Lane, Peter over 8 years ago

Ah, enum doesn't create it's own scope. Rats! Ok, it appears that I've lost this battle. Do what you need to do.

#15

Updated by Rogers, Chris over 8 years ago

Is this now closed?

#16

Updated by Taylor, Ian over 8 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100
  • Workflow changed from Awaiting Merge to Closed

Sorry, yes it is now closed.

#17

Updated by Rogers, Chris over 8 years ago

  • Target version changed from Future MAUS release to MAUS-v0.5.1

Also available in: Atom PDF