Repository navigation
Semisupervised learning updates - #184
stmartineau99 wants to merge 39 commits into
Conversation
constantinpape
left a comment
There was a problem hiding this comment.
Thanks for this PR! The integration of the micromatch backbones is a good step. Please address all inline comments. These are some bugs (two of them I could reproduce), redundant code paths, and smaller issues.
General issues without an inline anchor
- Please bump the torch-em requirement in
setup.pyandenvironment.yamlto>=0.10.8(see the comment onseparate_backward). flake8 --max-line-length 120reports about 25 issues in the changed files: missing blank lines (E302), trailing whitespace, no newline at the end ofmodels.py, and lines that are too long. Please fix them.- No test covers the backbone path or the new
semisupervised_training. A small test with a tiny 2D patch andcheck=True(no download) would catch problems such as theraw_keyissue.
Code that should go into torch-em
Most of models.py is general ViT-UNETR code and not specific to synapses. torch-em already contains the related code: preprocess_vit_inputs, the backbone handling in UNETR, and pooch-based checkpoint download in the new cryozoo.py. I suggest that we move these parts upstream:
| Part | Reason |
|---|---|
get_unetr_model, _get_vit_type |
A small factory for UNETR2D/UNETR3D. micromatch and synapse-net can then share it, and we fix the embed_dim bug in one location. |
_get_dinov2_checkpoint, _get_dinov3_checkpoint, _get_microsam2_checkpoint, _get_checkpoint |
Weight registries and download belong next to the model code (same pattern as CryoZoo.get_checkpoint). The microSAM2 entry probably belongs in micro_sam. |
get_raw_transform(backbone) / clip_max |
preprocess_vit_inputs already defines the expected input range for each backbone. A get_vit_raw_transform(backbone) next to it keeps the two consistent. |
_init_microsam_decoder |
General decoder initialization from a pretrained checkpoint. micro_sam's get_unetr has similar logic. |
| Weak augmentation parameters | MeanTeacherAugmenters should accept an augmentation dict, so that we do not need to change the torch-em module global. |
get_unsupervised_loader (optional) |
A default_raw_loader with sample masks in torch-em could replace the manual ConcatDataset. |
The U-Net factories, get_supervised_loader (boundary and ignore-label setup) and the warmup + mean-teacher logic in semisupervised_training should stay in synapse-net.
|
Above is Claude's review, but I checked all it's findings and they make sense and should be addressed. I would first refactor the general-purpose things to |
- Replace the private _download_sam_model with the public micro_sam.util.models() registry. Fetch the decoder only when return_decoder_path is set. - Import micro_sam at module level in a try/except ImportError block. Raise a RuntimeError with install instructions when the sam backbone is used without micro_sam. - Set decoder_path to None for backbones without a pretrained decoder, so that init_decoder=True no longer raises UnboundLocalError. Co-Authored-By: Claude Opus 5.5 <[email protected]>
stmartineau99
left a comment
There was a problem hiding this comment.
Bug:
decoder_pathis only set forsamand forsam2withhvit_t_em_organelles. Fordinov2,dinov3and the othersam2models,return_decoder_path=TrueraisesUnboundLocalError. Thusget_unetr_model(..., init_decoder=True)crashes for a 2D dino model, and thedecoder_path is Nonewarning in_init_microsam_decodercannot occur. Please setdecoder_path = Nonefor these backbones.
Done in b7b2bd2.
…None - avoids conflicts if raw_transform is passed thru **kwargs
…lization used during inference can match in training - for use with ViT backbones - added arg to caller
constantinpape
left a comment
There was a problem hiding this comment.
Thanks for the updates! Most of the review points are fixed now. Please see the new inline comment about aug_dict.
Tests
Please add these tests in test/training (tiny synthetic data, few iterations, the same setup as test_training_cli.py):
semisupervised_trainingwith the U-Net: check that it creates the warmup checkpoint and the final checkpoint, and that a second run reuses the warmup checkpoint. Use araw_keyother than"raw".get_raw_transform: the output range for"sam"([0, 255]) and"dinov2"/"dinov3"([0, 1]),(None, None)forNone, and aValueErrorfor an unknown backbone.- One smoke test for the backbone path:
supervised_trainingwith a backbone for a few iterations on a 2D patch. If the weight download is too large for CI, skip this test there.
| }, | ||
| "geometrical": DEFAULT_WEAK_AUGMENTATIONS["geometrical"], | ||
| } | ||
| augmenters = MeanTeacherAugmenters(ndim=ndim, clip_max=clip_max, aug_dict=aug_dict) |
There was a problem hiding this comment.
The aug_dict argument of MeanTeacherAugmenters is not in torch-em yet (main / 0.10.8 has only ndim, teacher, student, clip_max). Until it is released, this call raises a TypeError for every call of mean_teacher_adaptation. This includes the default synapse_net.run_domain_adaptation CLI and semisupervised_training.
Please make this change part of the new torch-em PR. When that version is released, update the torch-em pin in setup.py and environment.yaml to it.
There was a problem hiding this comment.
I meant to leave this PR open until after the torch_em one is done, I will make a PR after the retreat.
|
In addition to the review above: I want to wait with merging this till the PR that moves the shared functionality to |
… instead of whole volume
I am working on the new torch-em PR: I have a comment about the generic decoder initialization (
One concern (brought up by Claude) is |
|
Regarding your question @stmartineau99 :
The As a general comment: we should align with @anwai98 on Monday to check which functionality should live where. |
…se unetr model factories added by torch-em PR #861
…so far just cryosiam) backbones
…so far just cryosiam) backbones
Initial PR
Goal
Reorganization
get_2d_modelandget_3d_modelChanges
semisupervised_learning.py
backboneandmodel_type- currently used to select UNETR backbone x model type (needs rework when other backbones are added)teacher_warmup_iterationssemisuperivsed_trainingis now just a wrapper aroundsupervised_training, which performs the warmup, andmean_teacher_adaptiondomain_adaptation.py
get_raw_transformselects the appropriateraw_transformfor that backboneget_raw_transformreturnsclip_maxwhich is used to ensure the augmentations stay in the correct rangetorch_em.util.load_model; previously loading from the checkpoint without passingmodelfailed due to UNETR Class not settinginit_kwargs(though I fixed this yesterday in torch-em with Anwai)Deprecated
semisupervised_trainingrenamed tosemisupervised_training_v0- I think nobody used this so it can be removedweak_augmentationsnot used anymore since with switched to the new mean teacher setup - but I kept it around in dataloaders.py just in caseNext Steps
Response to Comments