Closed Sllambias closed 6 months ago
@asbjrnmunk @jakobamb feel free to add things here and suggest other things.
An example of what I envision would be to change this method in the Preprocessor:
def run_sanity_checks(self, images, label, subject_id, imagepaths):
self.sanity_check_standard_formats(images, label, subject_id, imagepaths)
if isinstance(images[0], nib.Nifti1Image):
self.sanity_check_niftis(images, label, subject_id)
def sanity_check_standard_formats(self, images, label, subject_id, imagepaths):
assert len(images) > 0, f"found no images for {subject_id + '_'}, " f"attempted imagepaths: {imagepaths}"
assert (
len(images[0].shape) == self.plans["dataset_properties"]["data_dimensions"]
), f"image should be shape (x, y(, z)) but is {images[0].shape}"
if label is not None:
# make sure images and labels are correctly registered
assert images[0].shape == label.shape, (
f"Sizes do not match for {subject_id}" f"Image is: {images[0].shape} while the label is {label.shape}"
)
# Make sure all modalities are correctly registered
if len(images) > 1:
for image in images:
assert images[0].shape == image.shape, (
f"Sizes do not match for {subject_id}" f"One is: {images[0].shape} while another is {image.shape}"
)
def sanity_check_niftis(self, images, label, subject_id):
if label is not None:
assert np.allclose(get_nib_spacing(images[0]), get_nib_spacing(label)), (
f"Spacings do not match for {subject_id}"
f"Image is: {get_nib_spacing(images[0])} while the label is {get_nib_spacing(label)}"
)
assert get_nib_orientation(images[0]) == get_nib_orientation(label), (
f"Directions do not match for {subject_id}"
f"Image is: {get_nib_orientation(images[0])} while the label is {get_nib_orientation(label)}"
)
if len(images) > 1:
for image in images:
assert np.allclose(get_nib_spacing(images[0]), get_nib_spacing(image)), (
f"Spacings do not match for {subject_id}"
f"One is: {get_nib_spacing(images[0])} while another is {get_nib_spacing(image)}"
)
assert get_nib_orientation(images[0]) == get_nib_orientation(image), (
f"Directions do not match for {subject_id}"
f"One is: {get_nib_orientation(images[0])} while another is {get_nib_orientation(image)}"
)
To this:
from somewhere_in_yucca.maybe_called_lib.sanity_checks.images import sanity_check_standard_formats, sanity_check_niftis
def run_sanity_checks(self, images, label, subject_id, imagepaths):
sanity_check_standard_formats(images, label, subject_id, imagepaths)
if isinstance(images[0], nib.Nifti1Image):
sanity_check_niftis(images, label, subject_id)
I forgot... Is this ready for review? :-)
Very much so, I just didnt request a review because it's a pretty enormous PR so it was only if you "wanted" to
I think this looks really good! The PR is too large for me to go into detail as you said, but i think it is definitely a net improvement and thus i think we should get it merged :-)
One comment actually:
Would it maybe make sense to have one more level of hierachy in the folders? I find that we have a lot of folders, and it is hard for me to know where things are. Logically, i would really like to make a difference between orchestration (the magic!) and the functionality and this PR is a step towards this. But i think we could go even further.
This is just from my hip, so might nok make total sense, but i think you get the point at least:
* functional
* orchestration
- managers
- evaluation
- planning
- preprocessing
- task_conversion
- configuration
- callbacks
* training
- data_loading -> renamed to just data
- augmentation
- loss_and_optim [i guess losses could go into functional and this should just be optim]
- network_architectures
- YuccaLightningModule.py (content of lightning modules)
* documentation
* run
When reviewing this PR do yourself a favor and do not try to go through files changed. Lots are simply deleted (e.g. the deprecated folder) and even more are just new import statements because paths are different. Instead go to the branch and view the repo from that branch so you get an idea of what it looks like: https://github.com/Sllambias/yucca/tree/Return-of-the-Lib/yucca
Goals: Separate functionality from configuration. Make a distinction between the Yucca classes that combine functions and the functions themselves. The Yucca classes should contain the Yucca-logic that defines what we give to the functions, but they should not also define the functions as methods unless absolutely necessary.
To clean up:
Probably worthy of mention:
preprocess_case_for_inference
andreverse_preprocessing
are now functions that allow proper preprocessing and reversion of preprocessing during inference for lightning modules that do not wish to depend on any YuccaPreprocessors.all augmentations are available as functions
Training folder has been undone
Clear distinction between functional stuff that will just do what it says and the "yucca" things/classes that will wrap functions and possibly configure stuff for you. I.e. if you want do to things from scratch import from functional and you're safe if you want to inhering yucca stuff import from non-functional folders