DOC: Correct the migration guide — v1 plans do not load in v2 - #633
Open
RafaelPalomar wants to merge 2 commits into
Open
DOC: Correct the migration guide — v1 plans do not load in v2#633RafaelPalomar wants to merge 2 commits into
RafaelPalomar wants to merge 2 commits into
Conversation
The guide told users "the upgrade is seamless: opening a .lrp.fcsv through the normal load path routes it to vtkMRMLResectionPlanStorageNode::ReadFcsv". That path was retired with the v1 markups subsystem. As shipped, the reader advertises only .lrp.json and rejects anything else, and the storage node's own header states that legacy .lrp.fcsv files no longer load. This is the worst class of documentation defect: it concerns user data, and acting on it loses access to plans. A user reading the old text would upgrade, discard their v1 installation, and find their resections unopenable with no converter and no warning. Rewritten as a full migration guide rather than a resection-plan note: what carries over (imaging and segmentations), what does not (every Slicer-Liver node, all v1 plans), the module renames, and the scripting API table for the wrapper/carrier/display split. The prominent warning now says to keep a working v1 installation until every plan is either re-created or consciously abandoned. The hand-recovery path is kept but corrected: the earlier draft of this rewrite used a flat-index SetControlPoint(i, x, y, z) that does not exist. The real Python-wrappable entry point takes row and column, and SetControlGrid does not cross the wrap at all. Every class name, accessor and enum value in the tables was checked against `main` (v1) and `preview` (v2) rather than carried over from the previous text.
The rejection path for a legacy .fcsv told the reader that migration of pre-T2 scenes "is covered by vtkMRMLResectionPlanLegacyFcsvMigrationTest". No such test exists anywhere in the tree, and no migration exists for it to cover. A reader hitting this error and following the comment would look for reassurance that the case is handled, and find nothing. The comment now states plainly that there is no migration path and points at the guide that tells a v1 user what can be recovered by hand.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First docs slice. Both commits fix documentation that actively misleads, rather than documentation that is merely missing.
The migration guide promised an upgrade that does not happen
Docs/migrations/v1-to-v2.mdsaid, verbatim:That path existed during v2 development and was retired with the v1 markups subsystem. As shipped:
qSlicerLiverResectionsReader.cxx:139advertises onlyLiver resection plan (*.lrp.json); anything else is rejected with "Unsupported file extension".vtkMRMLResectionPlanStorageNode.h:107states outright: legacy.lrp.fcsvfiles no longer load.vtkSlicerLiverResectionsLogic::LoadResectionserrors on.fcsv.This is the worst class of documentation defect, because it concerns user data and acting on it causes loss. A v1 user reading the old text would upgrade, discard their v1 installation, and discover their resection plans are unopenable — with no converter, no batch tool, and no warning anywhere in the upgrade path.
The guide is rewritten from a resection-plan note into an actual migration guide: an at-a-glance carries-over table, what happens to v1 scenes (core nodes load, every Slicer-Liver node is dropped), the module renames, the scripting-API table for the wrapper/carrier/display split, and a pre-upgrade checklist. The warning now leads with keep a working v1 installation.
A code comment cited a test that does not exist
vtkSlicerLiverResectionsLogic.cxx:359told a reader hitting the rejection that migration "is covered byvtkMRMLResectionPlanLegacyFcsvMigrationTest". No such test exists anywhere in the tree, and no migration exists for it to cover. Someone debugging a failed legacy load would go looking for reassurance and find nothing.Verification
Every factual claim was checked against both branches rather than carried forward from the old text or from a draft:
main, absent onpreview.ResectionMargin/UncertaintyMargin(vtkGetMacro), and the v1 enumsInitialization/Deformation/CompletedandFlat/Curved.Init/Planning/Confirmed.vtkLiverSegmentsLogicconfirmed onmain..lrp.json,.vta.json,.vsd.json.One correction made during drafting: the hand-recovery snippet originally used a flat-index
SetControlPoint(i, x, y, z). That signature does not exist — the real Python-wrappable entry point isSetControlPoint(row, col, x, y, z), andSetControlGrid(const double*)deliberately does not cross the wrap. Publishing the flat-index version would have handed users broken recovery code at exactly the moment they were trying to rescue data.One correction to an earlier assumption of mine: SlicerVMTK is not a new v2 dependency —
git show main:CMakeLists.txtshows it, SegmentEditorExtraEffects and ExtraMarkups were all already in v1'sEXTENSION_DEPENDS. The only genuinely new hard dependency is SlicerLayerDM. The guide says so.Reviewer's map
Docs/migrations/v1-to-v2.md— full rewrite. The load-bearing section is ".lrp.fcsvfiles do not load in v2"; the rest is the surrounding context a v1 user needs.vtkSlicerLiverResectionsLogic.cxx— one comment, no logic.Still to come in this epic
Release notes (
Docs/releases/v2.0.0.md), the 6-stage user guide, the developer guide, and liftingDocs/index.mdout of its self-declared scaffold state. Kept separate so this correction is not held up behind them.Authored with assistance from Claude (Anthropic Claude Opus 5).