Software Peer review

Under:

General Information

Before adding a new code to the CVS repository, any Maker and or code needs to be peer reviewed. Please, consult the STAR coding and naming standards pages before even starting to design a new maker or code. This document will help you shape, through basic guidelines, your code structure and layout.

Code peer review is a process by which members of the collaboration, code developers themselves, are asked to review your code as per its fitness to be included in the standard STAR framework. We hope this process will bring to developers of maker useful inputs as per the reuse of existing classes, the integration of the code within a chain, its interaction with our IO model etc ...

The task of the reviewers

The basic peer review should address the following issues :

  1. General coding style and standard Naming convention
    • Naming convention must be followed
    • Classes must have destructor / constructor
    • Access to data members must be through access methods
    • Global variables usage (should be avoided, suggestions if used)
    • Constants, variables, method should have self-explanatory names
    • ...
    There are more guidelines in the STAR coding and naming standards document the reviewers should consider. Reviewers should send their recommendations and specific advices if standards are broken.
  2. The code layout i.e. The directory structure (is the documentation in the proper doc/ directory, the macros in the macros/ directory etc ...)
  3. The presence of documentation and if it was sufficient to understand what the intent of code was. Complete lack of documentation should be considered as a show stopper for the review.
  4. Code general suggestions and comments :
    • issues related to re-use of existing base classes
    • merging with existing or similar sub-systems developed classes etc ...).
    • Hardwired constants and suggestions (for example, a database approach for calibration constants, if applicable, is a good suggestion).
    • In case of simulation makers, the reviewers should pay particular attention to the GEANT geometry and how it relates to the reconstruction code and, for a sub-system in development mode, the geometry flexibility.
  5. Is the algorithm sound, any missing parts / improvements suggestions, approaches the developer should try or consider in future.
  6. General comments on readiness of the code to be inserted.
  7. The almost last and one of the mandatory requirements is for the code to compile (hum ... it should be never knows ...) and this, on at least the preferred platform (a code will NOT be added if it fails this basic requirement) but preferably the two supported platforms.
  8. Finally, can you run the code (always good to compile but if it crashes, it is not ready)? Does it seem to provide meaningful results within the provided example and/or instructions?

There are at least two peer reviewers per new code. The peer reviewers should agree that the code is important, should be included in the set of makers, is ready for deployment and, if there are more work to be done, should clearly state what is required in their views for the code to be functional. Suggestions for future improvements must be clearly stated and a road map for implementation offered to the developer . The reviewers should also explicitly sign-off on the code.

As a last note, a peer review is advisory to the STAR Software & Computing leader. In case of disagreement, a summary and ruling will be sent to the person reviewed (and reviewers) indicating the changes to be made. After a peer review is closed, a notice will be sent to starsoft so all (core team as well as other coordinators) are informed of the new incoming code.

Preparing your code for a peer review

You should first make sure that your code will pass the above criteria. Your code must be available to the reviewers from a public space. Please, check protection (g+r in NFS space) or acl (star rl in AFS).

When you feel ready to have your code reviewed, send an Email to STAR Software & Computing leader requiring a review process to take place along with a quick description of your maker / code and the status of your code (still in development mode, final version, why you would like to have it included to the official repository, etc ...). Avoid development code at all cost unless clear justifications.

For a meaningful review, please provide instructions (macro or chain) on how to run your code with all arguments specified (least work for the reviewers, should work as instructed). Possibly, how to verify the results are correct in case your code deals with Physics-ready structures is an asset. At minimum, we require the code compiles & run and that reviewers understand its purpose and documentation is adequate.

Members of the collaboration will be asked to take on this task within a day or two. Comments, answers and action items must be sent and communicated via Emails to all members of the peer review committee (they will appear in the initial Email starting the review process). After all reviewers have sent their contributions, you will be informed of the the critical items to be taken care off and an expected / suggested time frame for the insertion of the new code in our library.

Past peer reviews

Makers Developer / Contact Reviewers Date/Status
StPmdClusterMaker
StPmdDiscriminatorMaker
StPmdSimulatorMaker
StPmdUtil
Subhasis Chattopadyay Alexandre P. Suaide
Akio Ogawa
August 2002
StJetFinder Mike Miller David Hardtke
Victor Perevoztchikov
Gene Van Buren
August 2002
StPythiaEvent requested but not
created / delayed.
StBbcSimulationMaker Mikhail Kopytine Janet Seyboth
Subhasis Chattopadyay
September 2002
Several suggestions made as per the database interface.
StMinuitVertexMaker David Hardtke Lee Barnby
Zhangbhu Xu
Closed (not summarized)
StBichsel
StdEdxY2Maker
Yuri Fisyak Jeff Porter
Fabrice Retiere
Closed with action items + testing needed
StSecondaryVertexMaker Julien Faivre Spyridon Margetis
Gene Van Buren
Closed (not summarized)
StEmcMixerMaker
Addition to StAssociationMaker
Alex P. Suaide
Marcia Maria de Moura
Patricia Fachini
Maxim Potekhin
January 21st 2003
StEEmcUtil
StEEmcDbMaker
StEEmcCalibrationMaker
StEEmcSimulatorMaker
Jan Balewski Alex P. Suaide
Herb Ward
January 27th 2003
Hardwired values should be removed and replaced by a database approach.
StHitFilterMaker James Dunlop Jerome Lauret February 6th 2003
Argument passing to constructor should be changed (the hack violates code standards)
Work on extraneous filters plug-and-play
StFtpcMixerMaker Frank Simon Frank Geurts
Jerome Lauret
February 14th 2003
Issues were all addressed (global variables, documentation etc ...)
StTriggerDataMaker Akio Ogawa
Mirko Planinic
Jerome Lauret
Thomas Ullrich
June 2003
Initially added in the library, review missed a lack of destructor in reading mode (relying on the StEvent model too much). Was fixed by Victor in June and stabilized.
StTofpMatchMaker
StTofpNtupleMaker
Frank Geurts Thorsten Kolleger
Boris Hippolyte
August 7th 2003
StTofpNtupleMaker recognized to be an analysis maker moved to StTofPool.
Opened issue : some variables need to be moved in a database.
StSsdClusterMaker
StSsdEvalMaker
StSsdSimulationMaker

StSsdPointMaker
Christelle Roy Frank Laue
Yuri Fisyak
March 12th 2004
Closed (one reviewer not responding). Some code will need re-evaluation later. StSsdPointMaker mostly addressed and ready. One concern about dimension used in multiple codes (not defined as const or other method)
StPmdReadMaker
StPmdCalibConstMaker
Subhasis Chattopadyay Frank Simon
Piotr Zolnierczuk
Closed (not summarized)
StKinkMaker Camelia Mironov Jason C Web
Claude Pruneau
March 2nd 2004
Code lacks internal documentation (doxygen).
There is an issue with the Bfield calculation (recalculated for every event).
Code to be merged in StSecondaryVertexMaker library.
StTofrMatchMaker Xin Dong Thomas Ullrich
Thomas Dietel
March 8th 2004
Rather large histograms enabled by default (no external control)
Many assert() commented to remove.
Documentation needed.
StStarLogger Valeri Fine Dmitry Arkhipkin
Gene Van Buren
May 14th 2004
Done Nov 2004 with agreement that non-implemented methods (and unused) will be cleaned + doc needed, The configuration file would need to be relocated as well.
StTofCalibMaker Xin Dong Marcelo Munhoz
Javier Castillo
June 8th 2004
All requested changes applied.
StEventCompendiumMaker Manuel Calderon Jerome Lauret
Yuri Fisyak
June 9th 2004
No action items for this maker. The code was found to be concise and clear.
StSvtEmbeddingMaker Petr Chaloupka Mike Miller
Camelia Mironov
June 16th 2004
Chain options to be added
StHeavyTagMaker Manuel Calderon Jerome Lauret July 28th 2004
Fast-lane reviewed
StSsdDaqMaker Christelle Roy Jeff Landgraf
Marcelo Munhoz

Jerome Lauret
October 2004 – Closed March 2005.
Remaining hardwired values to consolidate, message severity all “high” to revisit.
StSpinDbMaker Jan Balewski Marcia Maria de Moura
Michal Daugherty
September 2005 – Requester abandoned the review although well on the way. Code not added to CVS.
Rich Scalers Eric Hjort Gene Van Buren
Jerome Lauret
December 2006
Internal review
THIS REVIEW WAS STILL OPENED - DEAD
StTpcTracker Pibero Kisa Djawotho
(Jan Balewski)
Jerome Lauret November 2005 – Beam background track scavenger. Discussed in a separate meeting and S&C meeting on the 17th. Decision was to provide hooks in ITTF for this.
StFtpcCalibMaker Janet Seyboth Renee Fatemi
Jason Webb
January 2006 – Closed in April with the following minor remaining issues:
Use of CassDef() not necessary for this class
Over-use of comparison to 0 in if statement could be written as if (a) and if(!a).
StIstHit Mike Miller Yuri Fisyak
Jerome Lauret
January 2006
Internal review. Only one comment about the use of messenger. Class at the end supported both HFT and IST.
StIstSim Willie Leight Maxim Potekhin
Adam Kocoloski

Valeri Fine
February 2006 – Forgot and re-visited May 2006
Some contentions and divergence of opinion on this. Would require a resolve and summary.
StTpcBeamBackMaker Pibero Kisa Victor Perevoztchikov
Yuri Fisyak
Fast lane on Agust 8th 2006
Closed August 16th
StEmcMixerMaker Jan Balewski
Adam Kocoloski
Victor Perevoztchikov
Jonathan Bouchet
Opened March 12th 2007
Done April 13th but closed July 12th.
StSsdFastSimMaker Jonathan Bouchet Helen Caines
Anthony Timmins
Requested June 27th 2007
Done August 8th 2007 – no remaining issues.
StTriggerUtilities Akio Ogawa
Renee Fatemi
Jerome Lauret Requested June 27th 2007 - done August 23rd. This suite of utilities was declared useful and moved under Renee Fatemi's responsibilities. It will be a "user" utility and hence, compilation and maintenance will follow this category. This did not need a formal peer review (not used in production).
StLaserAnalysisMaker Yuri Fisyak Gene V. Buren
Richard Witt
Requested Dec 3rd 2007 (used prior but not pushed to review). Closed on December 20th 2007.
StTofHitMaker Xin Dong Yuri Fisyak
Renee Fatemi
Requested February 26th 2008. Was previously named StTofEventMaker. Closed on March 11th 2008.
StTpcHitMaker Yuri Fisyak Akio Ogawa
Jerome Lauret
Requested March 13th 2008. This review was closed on May 27th 2008. Reviewer asked for explaination of what this is for (so not all clear) and this is the equivalent for TPX of St_tpcdaq_Maker and FCFMaker.
The documentation is missing.
StBTofUtil
StBTofHitMaker
Xin Dong Matt. Walker
Valeri Fine
Renee Fatemi
Requested January 15th 2009.
Closed on January 31st 2009 after a two pass comment / correction only. The remaining action items includes documentation (doxygen minimal documentation + follow-up longer documentation being worked on by a student as an action item of the TOF software review) and a reshape of the RTS includes (not within the purview of the TOF sub-system).
StMCFilter Victor Perevoztchikov Michael Betancourt
Frank Geurts
Initiated April 10th 2009, started 13th.
Review was closed on June 18th with documentation action item.
StBTofMatchMaker
StBTofCalibMaker
Frank Geurts Pibero Djawotho
Rashmi Raniwala

 
(re)initiated April15th 2009 [was previously requested by Xin Dong in February]. Started April 20th.

StBTofMatchMaker done on June 22nd.
StBTofCalibMaker done on September 22nd.
St_pp2pp_Maker Kin Yip Valeri Fine
Thomas Ullrich
Started on October 7th 2009
StTpcRSMaker Yuri Fisyak for the TPC sub-system Pibero Djawotho
Jan Balewski
Initiated October 14th 2009. Closed November 5th 2009.
Action items includes study of the code performance (now a factor of 12 to 13 slower than TRS) and cleaning for hard-coded constants.
StFmsDbMaker Akio Ogawa Matthew Walker
Dmitry Arkhipkin
Initiated October 14th 2009. Closed October 27th 2009.
Action item include revisiting the documentation especially the doxygen self-documenting information.
StVpdCalibMaker Frank Geurts Gene V. Buren
Oleksandr Grebenyuk
Requested October 27th, started 28th.
Closed November 9th 2009.
StTofSimMaker Frank Geurts
(Dylan Thein)
Akio Ogawa
Gerrit V. Nieuwenhuizen

Requested October 27th, started 28th.
StEmbeddingQAMaker Hiroshi Masui for the embedding team
Andrew Rose
Anthony Timmins

Requested September 15th, delayed start until November 3rd (priority scheduling).