Child pages
  • Response to Code Review 04-24-14
Skip to end of metadata
Go to start of metadata

Code Review Meeting for the Reciprocal Space Mapping project was held on 4/24/14. This page contains list of participants, key event timeline, and summary of issues that were discussed at the meeting. Unless stated otherwise, all reviewers agreed on issues listed below.

Note that the unedited compilation of notes from all reviewers contains more detailed description of various problems, as well as additional comments and suggestions for improvements.

Participants

Author: John Hammonds
Reviewers: Arthur Glowacki (Scribe), Tim Mooney, Sinisa Veseli (Moderator)
Observers: Nicholas Schwarz

Key Events

  • 2014-03-25: Sinisa sent out Review Meeting Notice.
  • 2014-04-17: John tagged the code and sent out additional instructions for running it (tag URL: https://subversion.xray.aps.anl.gov/RSM/rsMap3D/tags/2014-04_CODE_REVIEW/).
  • 2014-04-23: Each reviewer prepared their own list of issues for the Review Meeting.
  • 2014-04-24: Review Meeting was held @ 10:00am (C2200). Arthur prepared meeting notes.
  • 2014-04-25: Sinisa prepared review report outlining issues that may have to be addressed.
  • 2014-04-29: Followup Meeting. John Hammonds and Nicholas Schwarz

Issues 

Bugs/Defects
  1. File Tab: Cancel button calls uninitialized variable if pressed before loading.
    1. This item was noticed during work other tasks (SSG_000116-101).  The button was disabled on initialization.
  2. File Tab: Entering wrong number of arguments into "Detector ROI" or "Number of Pixels To Average" results with a generic "list index out of range" exception, rather than with a specific error message.
    1. Will address.
    2. Added SSG_000116-117 to address this issue.
    3. Closed issue 5/5/2014
  3. File Tab: Supplying wrong xml file for Detector config results with a "NoneType object has no attribute findall" error, rather than with a specific error message.
    1. This is a result of not analyzing the XML on reads and therefore not providing custom exceptions.  Work on this also addresses #6 below.  Work on this was handled in SSG_000116-111 & SSG_000116-112.  A package specific exception class was added (also addresses Design/Coding Best Style/Practices/Extensibility # 6 below) and two subclass added for Exceptions during loading if instrument and detector configuration files.  Messages have been added to address missing pieces of these files that replace the message stated above.
  4. File Tab: User's choice for input project file is not limited via filters.
    1. Added filters to the file inputs during work on SSG_000116-112 above.  This was a small change.  A separate task was not created for this.
  5. File Tab: GUI components are not disabled while loading configuration/data files.
    1. Will address. 
    2. Added SSG_000116-118 to address this.
    3. Closed this issue 4/30/2014
  6. File Tab: Loading incorrect xml file does not display error / warning message.
    1. This is attached to # 3 above.  Message handling mentioned there should address this issue.
  7. File Tab: "Cancel" button is always enabled, even if there is nothing that can be canceled.
    1. See item # 1 above.
  8. File Tab: Loading spec files copied to a different location without underlying images does not indicate any errors, even though application will not work.
    1. Will address.  Missing a catch on zero scans found. 
    2. Added SSG_000116-119 to address this
    3. Closed Issue 4/30/2014
  9. File Tab: There is no application feedback ("working...", "done") on the state of progress through the load operation.
    1. A progress bar has been added to show progress on loading/processing data.  See SSG-000116-114.
  10. File Tab: GUI does not permit both Bad Pixel File and Flat Field Correction, even though underlying code allows it.
    1. This is by design.  The user will select one or the other.  The flat field file would contain the bad pixel information.
  11. Process Data Tab: User is not notified about where processing output is being stored.
    1. Will address this. 
    2. Added SSG_000116-120 to address this issue.
    3. Closed issue 4/30/2014
  12. Data Range Tab: Supplying min argument that is greater than max argument does not display error message.
    1. Will address this.
    2. Added SSG_000116-121 to address the issue.
    3. Closed this issue 5/5/2014
  13. Data Range Tab: Reset button is always enabled, and does not seem to be doing anything other than converting integer to float.
    1. The reset button resets to the last apply.  If nothing has changed since the last apply, reset will do nothing.  Need to disable if there are no changes.  
    2. Added SSG_000116-122 to address the issue.
    3. Closed this issue 5/5/2014
  14. Scans Tab: Column values for theta, chi, etc, can be changed to strings.
    1. Will address.  
    2. Added SSG_000116-123 to address this issue.
    3. Closed this issue 4/31/2014
  15. Scans Tab:  Numbers ("X min", etc.) appear editable, but they are not.

    1. Will address. 
    2. Added SSG_000116-124 to address this issue.
    3. Closed this issue 4/31/2014.
  16. Process Data Tab: Processing cannot be cancelled.
    1. See SSG_000116-71.  A story to add this was planned for the current sprint but was not finished at the time of the meeting.  This is now complete.  To accomplish this, processing has been spawned to a separate task so that the thread can be flagged to cancel.  Also as part of this, a progress bar was added to the GUI to show the user that processing is proceeding and to indicate that it has finished.
  17. Image Window: It is not connected to the main window. If closed, it cannot be brought back.
    1. Will Address. 
    2. SSG_000116-125
  18. Image Window: Mouse action is unintuitive, with no obvious way to recover the initial or default orientation.

    1. Will look at addressing this.  SSG_000116-126
  19. Sector33SpecDataSource.py: If orientation is not found in the input spec file, corresponding variable is not set to None.

    1. Will address. 
    2. Added SSG_0001160-127 to address
    3. Closed issue 5/13/2014
  20. gridmapper.py and polemapper.py: Those two files appear to be the same.

    1. polemapper.py merged into gridmapper after the transforms were added.  This needs to be removed. 
    2. Added SSG_000116-128 to address this issue
    3. Issue closed 5/12/2014
Documentation/Installation/Usability/Examples
  1. User Guide: It is written in MS Word.
    1. Will replace this with a PDF which is more universal. 
    2. Added SSG_000116-129 to address this issue.
    3. Closed this issue 5/5/2014
  2. Confluence Examples Page/User Guide: Requirement for specific data directory layout with respect to spec file is not documented (i.e., copying spec files into different location does not work).
    1. Have added some information on this to the Example page. 
    2. See SSG_000116-130
    3. Closed issue 5/13/2014
  3. Confluence Examples Page/User Guide: Documentation does not clearly outline what is needed to run the software with sample spec files.
    1. Will address
    2. Added SSG_000116-130 to address this.
    3. Closed issue 5/13/2014
  4. Confluence Installation Page: Required python packages do not have links.
  5. Confluence Installation Page: Required python version and versions for all dependencies are not listed.
  6. Confluence Installation Page: Several python package dependencies are not listed.
  7. Confluence Installation Page: Installation notes for windows are out of date.
    1. Will Address
    2. Have added SSG_000116-169 to address.
    3. Closed this issue 5/12/2014
  8. Confluence Installation Page: Explicit software installation instructions do not exist for linux/mac.
    1. Will address -
    2. Have added SSG_00116-166 to address this.
    3. Closed issue 5/12/2014
  9. Installation (RHEL 6.x): Software cannot be installed on RHEL 6.x using standard OS python packages (due to old version of numpy).
  10. Installation (Darwin 13.x): PySide namespace may be used instead of PyQt4 namespace.
  11. Installation (Fedora 20): "Image" module is under PIL namespace.
  12. Application Runtime: Data processing typically results in very large memory consumption (> 5GB at times), and gui becomes unresponsive.
    1. Memory issues will be examined at a later time.  For now, memory usage is known and accepted.  GUI responsiveness was due to processing being done in the GUI thread.  This was planned and has been addressed in JIRA story SSG_000116-71.
  13. Application Runtime: Application cannot be stopped using CTRL-C from the command line.
    1. Added SSG_000116-176 to address this.
    2. Closed issue 5/14/2014.
  14. Application Runtime: Several errors appear as exceptions on the terminal, without alerting user on the GUI screen that something went wrong.
    1. Will continue to work on eliminate these as they are found. 
    2. SSG_000116-131
    3. Went through code looking for more of this after all other work is completed.  Closed issue 5/13/2014
  15. Application Runtime: Files named "sys?" and "traceback?" appear on the filesystem (Note: only Tim experienced this problem.)
    1. Have not seen this.  Will talk with Tim.. 
    2. Added SSG_000116-132 to address this.
    3. Talked with Tim.  This occured when Tim was trying to directly execute the python file rsmEdit.py.  This was interpreted as bash and the import commands exist in bash and caused the files to appear.
  16. Application Runtime: Some input files (e.g. "YSZ111_1.spec") result in the "QObject::connect: Cannot queue arguments of type 'Qt::Orientation'" error.
    1. This was due to accessing GUI objects during the load file thread.  This has been addressed as I was adding threading to the Process tab (SSG_000116-85).
Design/Coding Style/Best Practices/Extensibility
  1. rsmEdit.py (line 100): Generic exception caught in load always throws file error, which may mask other error types.
    1. Have added a base class for exceptions in this package.  A few subclasses have been added and this will be reflected in try catch blocks as necessary.  See SSG_000116-112.
  2. fileform.py (line 6): Statement imports all names from PyQt4.QtCore package. Similar issues can be found elsewhere.
    1. Have addressed this during other work.
  3. fileform.py: Misspelled words in several comments.
    1. Will address.
    2. SSG_000116-134
    3. Closed this issue 5/9/2014
  4. Several classes and class methods have empty documentation strings.
    1. Will continue to address this throught the sprint.
    2. Have added SSG_000116-134 to address this.  Will go through the code at the end of the sprint to pick up straglers.
    3. Closed this issue 5/13/2014
  5. Code documentation does not list author/contact information (e.g., "APS/Software Services Group").
    1. Will add to license file. 
    2. Have added SSG_000116-135 to address this.
    3. Closed this issue 5/5/2014
  6. Project specific exceptions do not have common base.

    1. Have added a base class for package exceptions.  Have added a few subclasses for errors while loading file.
  7. Common strings that repeat are not constants (e.g. 'images/%s').

    1. Will address this. 

    2. Have added SSG_000116-136 to address this issue.

    3. Closed this issue 5/9/2014

       

       

       

  • No labels