The Slicer core team is working towards Slicer 5.0 and in the process removing functionality marked as deprecated. Generally, this is very much appreciated! I am maintaining some of our older extensions and want to make sure that they remain functional. Is there somewhere a list of all deprecated code pieces and a timeline by what date they will be removed?
I am asking because of the following two issues:
A few weeks ago the the vtkMRMLFiducialListNode was removed from Slicer, which broke one of our extensions (PETTumorSegmentation). I’m currently re-working the code to use the vtkMRMLAnnotationControlPointsNode instead, but as of right now, the extension is not functional.
Thanks for brining this up - we do need to get a better handle on the deprecation process and also communicate how to migrate so that extensions can be better maintained. It has also happened to me that I found that an extension needed to be updated, but there were no documents I could find that described how to migrate (just a note to “use class XX instead”). Definitely let’s post when such things come up so there’s a record.
For your first issue, I hope it’s just a matter of renaming to follow this commit.
For the second, that appears to be a 7 year old comment, so maybe it was pre-mature - I don’t think that code is likely to change soon. But maybe others can comment.
Giving sufficient time to extension maintainers is important for the health and sanity of the community.
In the short term, should we consider re-integrating the vtkMRMLFiducialListNode ?
Background
We aim to communicate about it by:
documenting removal of feature on the Roadmap (also linked from Slicer mediawiki here)
creating issue identifying which extensions need to be updated. For example, see here for removal of “Editor” and “Charts” legacy modules
The removal of the Slicer3 MRML nodes without identifying which extension would be impacted was an oversight (only relying on the fact the class was marked deprecated in June 2012) was probably not sufficient.
vtkMRMLFiducialListNode was removed based on the deprecation from June 2012 which means it had been deprecated for 11.5 years which seems reasonable to assume it has been a long enough period for developers to transition to the replacement.
It was removed from Slicer at this point because it was making development of the most recent replacement, the “Markups” module, more difficult. It seems at a point when maintaining very old code becomes a hindrance of latest development is a good point for removal to reduce burden of developers.
Latest efforts have been marking methods deprecated with runtime warnings to make it clear to developers that the used method should be replaced by the suggested method. Providing deprecation for vtkMRMLFiducialListNode was difficult because the replacement vtkMRMLAnnotationFiducialNode is itself deprecated as of Sept 2013. The latest node object that should be used is the vtkMRMLMarkupsFiducialNode.
Although the recent work makes it clearer to developers what methods are deprecated and provides a suggestion about what methods should be used instead, it doesn’t specify a time when it will be removed from the code. I’m not aware of a code convention to predefine when something is going to be removed after being deprecated. Usually it seems that once deprecated it is subject for removal at any point in the future. Some deprecated items in Slicer have potential removal dates as stated in Roadmap · Slicer/Slicer Wiki · GitHub such as the Annotations module which includes vtkMRMLAnnotationFiducialNode to be removed in May 2023. Do we need to add more dates to everything deprecated to this Slicer roadmap, or do we need insert dates into the code where it automatically goes from being say a runtime warning to a runtime error once it reaches a certain date?
The Slicer developer team is generally doing a great job communicating changes affecting others ahead of time. Like the heads-up about removing the Legacy Editor was excellent. It was posted as part of the Roadmap and we even received an email that this is coming soon and affecting our extension. We had plenty of time to plan when to transition our code. I can see that the Slicer team is taking this seriously.
In the short term, should we consider re-integrating the vtkMRMLFiducialListNode ?
I am almost done transitioning our code to vtkMRMLAnnotationControlPointsNode. But I don’t know if this change also affected others or not. Generally, I also agree that this class should get removed from Slicer.
Regarding the deprecated DICOMLoadable. It has also been marked deprecated for 7 years or so already, but it’s not listed for removal in the Roadmap. Can I assume that it won’t get removed in the next couple months?
Do you plan to remove any other deprecated code before releasing Slicer 5.0 that is not mentioned in the Roadmap?
@chribaue Is there a reason you have decided to transition from vtkMRMLFiducialListNode to vtkMRMLAnnotationControlPointsNode which is also deprecated and scheduled for removal in a year? You will probably get many runtime warnings using that method which are pointing you to use vtkMRMLMarkupsFiducialNode instead. I wouldn’t want you to waste effort transitioning to another deprecated object that will have to be fixed again shortly (maybe 1 year timeframe).
Yes, the second point can be ignored @chribaue . My memories from when I added that note are a bit blurry after so many years, but I think in that early stage of the DICOM export / SH work we intended moving the core of the DICOM examine infrastructure to C++. Then we realized that there are Python-only usages and very different scenarios, and we never removed it. Instead, as far as I remember, we added both Qt and VTK counterparts for this Python class and converters among them.
I can remove this note to prevent future confusion because as I see now the Python DICOMLoadable class is not intended to be deprecated.
We added qSlicerDICOMLoadable to have a class that can be conveniently used from both C++ and Python and this meant that the Python-only DICOMLoadable could be removed. Removing the Python class would have reduced code duplication, but it would also have taken away the ability of Python scripted DICOM importers to save arbitrary Python objects into the DICOMLoadable class (no members can be added dynamically to a C++ class). This extra flexibility has been proven to be useful, so it is worth having the duplicate Python and C++ code and therefore the Python class should not be deprecated. @cpinter it would be great if you could update the documentation of the class, explaining the rationale for duplication.
If such needs arise often then we should introduce a design for dual-personality Python/C++ classes, which are implemented in C++ but can be monkey-patched like Python classes. Having a self() member to store a Python implementation class (as we do it in many Python scripted C++ class implementation) works, but is somewhat cumbersome to use in Python.