Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix vtkITKArchetypeImageSeriesReader::CanReadFile() #1269

Closed

Conversation

Sunderlandkyl
Copy link
Member

vtkITKArchetypeImageSeriesReader::CanReadFile previously returned true as long as the Archetype was set, and the file exists.
Now, CanReadFile will return true if, in addition to the archetype being set and the file existing, an itk::ImageFileReader can be instantiated for the specified file.

@pieper
Copy link
Member

pieper commented Nov 26, 2019

Makes sense to me. Was there a specific issue that requires this change? (I.e. wouldn't it just have failed later?).

@Sunderlandkyl
Copy link
Member Author

It was causing some failing tests in SlicerRT. vtkMRMLSegmentationStorageNode would try to load polydata as an image and report an error. Everything still loaded fine, but the error messages caused the test to fail.

Probably there is some other way to work around the error messages, but I figured that the the better approach would be to determine if vtkITKArchetypeImageSeriesReader can actually read the file before attempting to use it.

@pieper
Copy link
Member

pieper commented Nov 26, 2019

Yes, makes sense. Fine to merge from my point of view.

@jcfr
Copy link
Member

jcfr commented Nov 26, 2019

I wonder if there is anyway to refactor the code used in both RequestInformation and CanReadFile into an helper function ?

@pieper
Copy link
Member

pieper commented Nov 26, 2019

Yes, all that code is pretty messy. Could make sense to take a fresh pass at the whole class.

@jcfr
Copy link
Member

jcfr commented Nov 26, 2019

This could be done later of course and I would have no issue integrated this patch, just wanted to point it out.

@Sunderlandkyl I will let you assess.

@Sunderlandkyl
Copy link
Member Author

Sunderlandkyl commented Nov 26, 2019

I agree, it makes sense to reduce the code duplication.
There's no hurry to integrate, so I'll make the changes in this PR.

@jcfr
Copy link
Member

jcfr commented Nov 26, 2019

so I'll make the changes in this PR.

🙏

@jcfr jcfr added the wip label Nov 26, 2019
@jcfr jcfr changed the title BUG: Fix vtkITKArchetypeImageSeriesReader::CanReadFile() [Do not integrate] BUG: Fix vtkITKArchetypeImageSeriesReader::CanReadFile() Nov 26, 2019
@Sunderlandkyl
Copy link
Member Author

I've added a GetImageIO function that is used by both RequestInformation and CanReadFile.

vtkITKArchetypeImageSeriesReader::CanReadFile previously returned true as long as the Archetype was set, and the file exists.
Now, CanReadFile will return true if an itk::ImageFileReader can be instantiated for the specified file.
@Sunderlandkyl
Copy link
Member Author

@jcfr @pieper @lassoan Let me know if there are any other necessary changes.

@lassoan
Copy link
Contributor

lassoan commented Jan 11, 2020

The title of this PR is still "do not integrate" - can you confirm that it can be integrated now? Thank you!

@Sunderlandkyl Sunderlandkyl changed the title [Do not integrate] BUG: Fix vtkITKArchetypeImageSeriesReader::CanReadFile() BUG: Fix vtkITKArchetypeImageSeriesReader::CanReadFile() Jan 12, 2020
@Sunderlandkyl
Copy link
Member Author

@lassoan It should be ready to integrate.

@cpinter
Copy link
Member

cpinter commented Jan 12, 2020

Thanks a lot, everyone! I integrated it (Slicer/Slicer@8ea6945) so that we can see in the next nightly if the SlicerRT tests pass, to make progress with this PR.

@cpinter cpinter closed this Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants