acil-bwh / SlicerCIP

Slicer extension for the Chest Imaging Platform
BSD 3-Clause "New" or "Revised" License
15 stars 22 forks source link

Fix memory leaks, warnings and errors and also add qt5 support #21

Closed jcfr closed 6 years ago

jcfr commented 6 years ago

@rjosest While testing the recent updates associated with Slicer build system, I ended updating your extension

(also thanks for reviewing and integrating https://github.com/acil-bwh/ChestImagingPlatform/pull/21)

That said, it would be great if you could review this set of changes.

Worth noting that when building against Slicer+Qt4+VTK7, all tests are passing (expect py_CIP_LesionModel because of #16 and py_CIP_LesionModel because of #20). See report below for more details.

Note that while I didn't implement more tests, I marked the one that were not implemented as passing to avoid false negative (aka false failure) (see #17, #18, #19)

Before integrating, let's wait for https://github.com/Slicer/Slicer/pull/785 to be integrated into Slicer master. @pieper commit https://github.com/acil-bwh/SlicerCIP/commit/26c962c3e6d858e33c43772dbbb2dc4ecdcc5dd3 motivated the update of Slicer

It also build successfully against Slicer+Qt5+VTK8, that said the tests are not passing because SimpleITK is not available in that configuration (See https://github.com/SimpleITK/SimpleITK/issues/260)

Let us know with you have any questions. The Slicer discussion forum is a good outlet for questions.

As a side note, the migration guide associated with VTK6, VTK7, VTK8, Qt5 and Slicer are now all linked of https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/MigrationGuide

Test results:

$ ctest -j10
Test project /home/jcfr/Projects/SlicerCIP-build/inner-build
      Start  8: py_CIP_LesionModel
 1/23 Test  #8: py_CIP_LesionModel ........................................................***Failed    3.96 sec
      Start 10: py_CIP_MIPViewer
 2/23 Test #10: py_CIP_MIPViewer ..........................................................   Passed    4.86 sec
      Start 12: py_CIP_PAARatio
 3/23 Test #12: py_CIP_PAARatio ...........................................................***Failed    4.80 sec
      Start 15: py_CIP_ParenchymaSubtypeTraining
 4/23 Test #15: py_CIP_ParenchymaSubtypeTraining ..........................................   Passed    5.08 sec
      Start 17: py_CIP_TracheaStentPlanning
 5/23 Test #17: py_CIP_TracheaStentPlanning ...............................................   Passed    6.02 sec
      Start  6: py_CIP_BodyComposition
 6/23 Test  #6: py_CIP_BodyComposition ....................................................   Passed   58.35 sec
      Start  2: py_CIP_AVRatio
 7/23 Test  #2: py_CIP_AVRatio ............................................................   Passed    6.00 sec
      Start  5: py_CIP_ParenchymaAnalysis
 8/23 Test  #5: py_CIP_ParenchymaAnalysis .................................................   Passed    3.87 sec
      Start 14: py_nomainwindow_qSlicerCIP_ParenchymaSubtypeTrainingModuleGenericTest
 9/23 Test #14: py_nomainwindow_qSlicerCIP_ParenchymaSubtypeTrainingModuleGenericTest .....   Passed    2.87 sec
      Start 16: py_nomainwindow_qSlicerCIP_TracheaStentPlanningModuleGenericTest
10/23 Test #16: py_nomainwindow_qSlicerCIP_TracheaStentPlanningModuleGenericTest ..........   Passed    2.87 sec
      Start 13: py_nomainwindow_qSlicerCIP_PointsLabellingModuleGenericTest
11/23 Test #13: py_nomainwindow_qSlicerCIP_PointsLabellingModuleGenericTest ...............   Passed    3.78 sec
      Start 21: qSlicerRegionTypeModuleWidgetGenericTest
      Start 19: qSlicerAirwayInspectorModuleWidgetGenericTest
      Start 18: qSlicerAirwayInspectorModuleGenericTest
      Start 20: qSlicerRegionTypeModuleGenericTest
      Start 22: qSlicerParticlesDisplayModuleGenericTest
      Start 23: qSlicerParticlesDisplayModuleWidgetGenericTest
12/23 Test #22: qSlicerParticlesDisplayModuleGenericTest ..................................   Passed    2.81 sec
13/23 Test #23: qSlicerParticlesDisplayModuleWidgetGenericTest ............................   Passed    2.81 sec
14/23 Test #18: qSlicerAirwayInspectorModuleGenericTest ...................................   Passed    3.14 sec
15/23 Test #19: qSlicerAirwayInspectorModuleWidgetGenericTest .............................   Passed    3.14 sec
16/23 Test #20: qSlicerRegionTypeModuleGenericTest ........................................   Passed    3.22 sec
17/23 Test #21: qSlicerRegionTypeModuleWidgetGenericTest ..................................   Passed    3.43 sec
      Start 11: py_nomainwindow_qSlicerCIP_PAARatioModuleGenericTest
18/23 Test #11: py_nomainwindow_qSlicerCIP_PAARatioModuleGenericTest ......................   Passed    2.97 sec
      Start  9: py_nomainwindow_qSlicerCIP_MIPViewerModuleGenericTest
19/23 Test  #9: py_nomainwindow_qSlicerCIP_MIPViewerModuleGenericTest .....................   Passed    2.97 sec
      Start  3: py_nomainwindow_qSlicerCIP_InteractiveLobeSegmentationModuleGenericTest
20/23 Test  #3: py_nomainwindow_qSlicerCIP_InteractiveLobeSegmentationModuleGenericTest ...   Passed    2.87 sec
      Start  7: py_nomainwindow_qSlicerCIP_LesionModelModuleGenericTest
21/23 Test  #7: py_nomainwindow_qSlicerCIP_LesionModelModuleGenericTest ...................   Passed    2.96 sec
      Start  1: py_nomainwindow_qSlicerCIP_AVRatioModuleGenericTest
22/23 Test  #1: py_nomainwindow_qSlicerCIP_AVRatioModuleGenericTest .......................   Passed    2.86 sec
      Start  4: py_nomainwindow_qSlicerCIP_ParenchymaAnalysisModuleGenericTest
23/23 Test  #4: py_nomainwindow_qSlicerCIP_ParenchymaAnalysisModuleGenericTest ............   Passed    2.96 sec

91% tests passed, 2 tests failed out of 23

Label Time Summary:
qSlicerAirwayInspectorModule     =   6.28 sec (2 tests)
qSlicerParticlesDisplayModule    =   5.61 sec (2 tests)
qSlicerRegionTypeModule          =   6.65 sec (2 tests)

Total Test time (real) = 123.52 sec

The following tests FAILED:
      8 - py_CIP_LesionModel (Failed)
     12 - py_CIP_PAARatio (Failed)
Errors while running CTest
rjosest commented 6 years ago

@jcfr Thanks a lot for those fixes. Everything looks good from our end.

jcfr commented 6 years ago

Great. Thanks for the review.