search

QIICR / QuantitativeReporting

Segmentation-based measurements with DICOM import and export of the results.
https://qiicr.gitbooks.io/quantitativereporting-guide
Other
23 stars 23 forks source link

segmentation export broken in 4.10.2 #240

Closed pieper closed 5 years ago

pieper commented 5 years ago

I did the following with 4.10.2 (on mac):

The error log shows this:

  File "<string>", line 2, in <module>
  File "/Applications/Slicer-4.10.2.app/Contents/Extensions-28257/QuantitativeReporting/lib/Slicer-4.10/qt-scripted-modules/DICOMSegmentationPlugin.py", line 345, in export
    exporter.export(exportable.directory, segFileName)
TypeError: export() takes at least 4 arguments (3 given)

It looks like the signature of the export method changed but the call to export did not, so now there's an argument mismatch.

https://github.com/QIICR/QuantitativeReporting/commit/e64b310f9ab1d4fab553ea5ee6c69984040f5596

pieper commented 5 years ago

@fedorov @che85 any suggestions?

fedorov commented 5 years ago

Indeed, looks like it. I don't recall if I ever used that export feature. I will work on fixing it tomorrow. Thank you for the report!

fedorov commented 5 years ago

Does either of you know if that metadata parameter should be passed down somewhere from the SubjectHierarchy?

pieper commented 5 years ago

No, I don't know. Maybe @cpinter can help?

fedorov commented 5 years ago

From the quick look in https://github.com/Slicer/Slicer/blob/master/Modules/Scripted/DICOMPlugins/DICOMScalarVolumePlugin.py#L518-L535, I am thinking to check for those attributes with exportable.tag(), and if the result is empty, initialize to the default value in the plugin.

fedorov commented 5 years ago

@pieper I tested and pushed fixes to master and 4.10 - please reopen if you see something is still wrong!

pieper commented 5 years ago

Previous error is gone 👍

But now I get this (looks like same issue as #243 but in a different part of the code)

Traceback (most recent call last):
  File "<string>", line 2, in <module>
  File "/private/tmp/Slicer.app/Contents/Extensions-28388/QuantitativeReporting/lib/Slicer-4.11/qt-scripted-modules/DICOMSegmentationPlugin.py", line 350, in export
    exporter.export(exportable.directory, segFileName, metadata)
  File "/private/tmp/Slicer.app/Contents/Extensions-28388/QuantitativeReporting/lib/Slicer-4.11/qt-scripted-modules/DICOMSegmentationPlugin.py", line 454, in export
    data = self.getSeriesAttributes()
  File "/private/tmp/Slicer.app/Contents/Extensions-28388/QuantitativeReporting/lib/Slicer-4.11/qt-scripted-modules/DICOMSegmentationPlugin.py", line 541, in getSeriesAttributes
    seriesNumber = ModuleLogicMixin.getDICOMValue(volumeNode, DICOMTAGS.SERIES_NUMBER)
  File "/private/tmp/Slicer.app/Contents/Extensions-28388/SlicerDevelopmentToolbox/lib/Slicer-4.11/qt-scripted-modules/SlicerDevelopmentToolboxUtils/decorators.py", line 194, in __call__
    raise TypeError("no match for types %s" % str(types))
TypeError: no match for types ('vtkMRMLScalarVolumeNode', 'str')
fedorov commented 5 years ago

What are the steps to reproduce? I did test with the patched code, and I was able to export segmentation created in SegmentEditor using SH export function in 4.10.

pieper commented 5 years ago

I should have noted: these issues (#240 and #243) arise in the nightly (just confirmed with today's Slicer and corresponding QR extension). Both appear to be python3-related.

Steps to reproduce this one are as described in the first post on this issue.

fedorov commented 5 years ago

Thanks - I can reproduce it with nightly. I did test with a nightly by copying the modified script to the package directory before committing the changes, and did not get this error - no idea how this works.

Will look into it.

@che85 do you have any thoughts?

fedorov commented 5 years ago

I spent some time looking, but I am missing a lot of background on what's going on.... Will wait for @che85 to hopefully respond before I spend much more time on this.

che85 commented 5 years ago

I might have time to look into this later today/tonight

che85 commented 5 years ago

I didn't find time yesterday, but I think I know what's going on.

If you take a look at https://github.com/QIICR/SlicerDevelopmentToolbox/blob/7eaa5eeb785d91212ff69e9324eb973b488293f3/SlicerDevelopmentToolboxUtils/mixins.py#L713-L757

You will notice that there are multiple getDICOMValue methods implemented that are decorated with input types. What's happening in the error message is a sign for the wrong input types.

I will do some testing on this.

fedorov commented 5 years ago

Yes, I think I realized that, but could not quite figure out why it is implemented that way, or why python3 switch would cause a problem. From the past experience, unicode is gone as a type, maybe this is somehow related?

che85 commented 5 years ago

Yeah, something is wrong there. Let me look into it.

che85 commented 5 years ago

I am not sure, but I think six or whatever you were using for migrating code to py3 messed some things up. I wouldn't have done that. It might be convenient at first but now you have to debug everything. I fixed this error but then you get new ones for other calls.

fedorov commented 5 years ago

I am fine rolling back those six commits. I thought that was the right way to do it.

che85 commented 5 years ago

I mean, six is nice to have. It might just be the way I implemented things. Let me think about it again. Don't merge my PR yet.

fedorov commented 5 years ago

Sure. Also, to be clear, six was added by the python-modernize script, I did not add it manually. I thought using that script is more reliable, since it presumably addresses issues without having to wait for a runtime error to be reported. Apparently, it is not as simple as that. You indeed the way you implemented things is beyond what that package can do.

che85 commented 5 years ago

Yeah, I implemented this way because I didn't want to have different method names for different input types even though they are basically doing the same but slightly different. Maybe I was wrong with doing that but for me, it's much better to read the code (like C++ templates)

fedorov commented 5 years ago

When I was debugging this, I stopped when I saw the content of typemap here https://github.com/QIICR/SlicerDevelopmentToolbox/blob/master/SlicerDevelopmentToolboxUtils/decorators.py#L188:

{('vtkMRMLMultiVolumeNode', 'unicode', 'str'): <function getDICOMValue at 0x13c596398>, ('vtkMRMLScalarVolumeNode', 'str', 'str'): <function getDICOMValue at 0x13c596398>, ('FileDataset', 'str', 'unicode'): <function getDICOMValue at 0x13c581f50>, ('str', 'unicode', 'str'): <function getDICOMValue at 0x13c5961b8>, ('str', 'str', 'str'): <function getDICOMValue at 0x13c5961b8>, ('vtkMRMLMultiVolumeNode', 'str'): <function getDICOMValue at 0x13c5962a8>, ('unicode', 'str', 'str'): <function getDICOMValue at 0x13c5961b8>, ('vtkMRMLScalarVolumeNode', 'unicode', 'unicode'): <function getDICOMValue at 0x13c596398>, ('vtkMRMLMultiVolumeNode', 'unicode', 'unicode'): <function getDICOMValue at 0x13c596398>, ('FileDataset', 'str'): <function getDICOMValue at 0x13c581e60>, ('str', 'unicode', 'unicode'): <function getDICOMValue at 0x13c5961b8>, ('FileDataset', 'str', 'str'): <function getDICOMValue at 0x13c581f50>, ('unicode', 'str'): <function getDICOMValue at 0x13c5960c8>, ('str', 'str', 'unicode'): <function getDICOMValue at 0x13c5961b8>, ('vtkMRMLMultiVolumeNode', 'unicode'): <function getDICOMValue at 0x13c5962a8>, ('unicode', 'unicode', 'unicode'): <function getDICOMValue at 0x13c5961b8>, ('unicode', 'unicode', 'str'): <function getDICOMValue at 0x13c5961b8>, ('vtkMRMLMultiVolumeNode', 'str', 'unicode'): <function getDICOMValue at 0x13c596398>, ('FileDataset', 'unicode'): <function getDICOMValue at 0x13c581e60>, ('str', 'unicode'): <function getDICOMValue at 0x13c5960c8>, ('FileDataset', 'unicode', 'str'): <function getDICOMValue at 0x13c581f50>, ('vtkMRMLScalarVolumeNode', 'unicode', 'str'): <function getDICOMValue at 0x13c596398>, ('vtkMRMLScalarVolumeNode', 'unicode'): <function getDICOMValue at 0x13c5962a8>, ('vtkMRMLMultiVolumeNode', 'str', 'str'): <function getDICOMValue at 0x13c596398>, ('vtkMRMLScalarVolumeNode', 'str', 'unicode'): <function getDICOMValue at 0x13c596398>, ('str', 'str'): <function getDICOMValue at 0x13c5960c8>, ('FileDataset', 'unicode', 'unicode'): <function getDICOMValue at 0x13c581f50>, ('unicode', 'unicode'): <function getDICOMValue at 0x13c5960c8>, ('unicode', 'str', 'unicode'): <function getDICOMValue at 0x13c5961b8>, ('vtkMRMLScalarVolumeNode', 'str'): <function getDICOMValue at 0x13c5962a8>}

That was too much to digest for me! I realized I am missing the background to understand why and what this means... :-\

che85 commented 5 years ago

Yeah at first glance it looks complicated. It's a dictionary that maps all defined input types to the decorated function. 

@multimethod([slicer.vtkMRMLScalarVolumeNode, "vtkMRMLMultiVolumeNode"], str)
def somemethod(input, tag):
    # do something

This code would register somemethod for 2 different variants:

  1. somemethod(input: slicer.vtkMRMLScalarVolumeNode, tag: str)
  2. somemethod(input: "vtkMRMLMultiVolumeNode", tag: str)

Could reimplement the whole thing without this functionality though

che85 commented 5 years ago

Alright, I removed the decorator from the toolkit and replaced getDICOMValue with a different implementation which probably is much easier to understand. Let me know if there is anything else.

fedorov commented 5 years ago

Christian, you did this in your fork, and your fork is 124 commits behind the master!

che85 commented 5 years ago

No it's not behind:

https://github.com/che85/SlicerDevelopmentToolbox/tree/fix_multimethod

fedorov commented 5 years ago

Sorry, I was confused! Let me test this.

fedorov commented 5 years ago

This import should be removed, right: https://github.com/che85/SlicerDevelopmentToolbox/blob/fix_multimethod/SlicerDevelopmentToolboxUtils/mixins.py#L9

But otherwise the error is gone. Will you make a PR?

che85 commented 5 years ago

I changed that and merged the PR

fedorov commented 5 years ago

Great, thanks a lot for your help!

Nitpick - this is issue 240, not 24. For completeness, the fix is in this commit: https://github.com/QIICR/SlicerDevelopmentToolbox/commit/407d02f5e2e4e7802f60df75e249c75e55a674e7.

@pieper please reopen #243 if the error in the nightly is still present.

pieper commented 5 years ago

fix is confirmed on today's nightly - thank you guys!