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

First time trying out the package #243

Closed
zwep opened this issue May 10, 2023 · 8 comments · Fixed by #250
Closed

First time trying out the package #243

zwep opened this issue May 10, 2023 · 8 comments · Fixed by #250
Labels
bug Something isn't working

Comments

@zwep
Copy link

zwep commented May 10, 2023

Describe the bug

When using the following .yaml file located here, and replacing the 'inference' section with

inference:
batch_size: 8
dataset:
name: CalgaryCampinas
crop_outer_slices: true
text_description: inference
transforms:
masking:
name: CalgaryCampinas
accelerations: [ 10 ]

Then when running the following command (I have left out the definition for each variable below, since I dont think they are relevant)

direct predict $result_path
--checkpoint $chpt_path
--cfg $cfg_path
--data-root $data_root
--num-gpus 1

I run into the following issue

File "/local_scratch/sharreve/anaconda3/envs/direct/lib/python3.9/site-packages/direct-1.0.5.dev0-py3.9-linux-x86_64.egg/direct/predict.py", line 22, in _get_transforms
transforms = build_inference_transforms(env, mask_func, dataset_cfg)
File "/local_scratch/sharreve/anaconda3/envs/direct/lib/python3.9/site-packages/direct-1.0.5.dev0-py3.9-linux-x86_64.egg/direct/inference.py", line 138, in build_inference_transforms
transforms = partial_build_mri_transforms(**remove_keys(dataset_cfg.transforms, "masking"))
TypeError: build_mri_transforms() got an unexpected keyword argument 'cropping'

To Reproduce
Install the direct package via cuda
Download the pretrained rim model
Unzip this and define the appropriate path names
Change the 'projects/calgary_campinas/configs/base_rim.yaml' with the suggestions above
Run the 'direct predict ...' command

Expected behavior
A succesful run

Environment
dlup version: Not installed.
direct version: 1.0.5.dev0
How installed: using conda
Python version: 3.9
Operating System:
NAME="Debian GNU/Linux"
VERSION_ID="11"
VERSION="11 (bullseye)"
VERSION_CODENAME=bullseye

Additional context
Add any other context about the problem here.

@zwep zwep added the bug Something isn't working label May 10, 2023
@georgeyiasemis
Copy link
Contributor

Hi @zwep.

Thanks for using DIRECT and reporting the above issues. To be able to help you better, please split each individual problem into seperate issues. Also please provide the configuration file you used.

@zwep
Copy link
Author

zwep commented May 10, 2023

Hi @zwep.

Thanks for using DIRECT and reporting the above issues. To be able to help you better, please split each individual problem into seperate issues. Also please provide the configuration file you used.

Hi, I have rephrased the original issue to focus only on one thing. I hope this helps!

@zwep
Copy link
Author

zwep commented May 10, 2023

Hi @georgeyiasemis

I have done some debugging.. and I come to the following adaptation to TransformConfig. However, given the changes that I made, I am really not sure if Im doing it right. I do get it working, but I have the feeling that something else is going wrong.
Anyhow, with this version of TransformConfig I can run it without getting errors

@dataclass
class TransformsConfig(NormalizationTransformConfig, RandomAugmentationTransformsConfig, SensitivityMapEstimationTransformConfig):
    masking: MaskingConfig = MaskingConfig()
    # SH: Changing cropping to crop since build_mri_transforms does not have cropping but crop
    # cropping: CropTransformConfig = CropTransformConfig()
    crop: CropTransformConfig = CropTransformConfig()
    # SH: Removing this, and added the RandomAugmentationTransformsConfig as main class
    # random_augmentations: RandomAugmentationTransformsConfig = RandomAugmentationTransformsConfig()
    padding_eps: float = 0.001
    estimate_body_coil_image: bool = False
    # SH: Removing this, and added SensitivityMapEstimationTransformConfig as main class
    # sensitivity_map_estimation: SensitivityMapEstimationTransformConfig = SensitivityMapEstimationTransformConfig()
    # SH: Again as above
    # normalization: NormalizationTransformConfig = NormalizationTransformConfig()
    delete_acs_mask: bool = True
    delete_kspace: bool = True
    image_recon_type: str = "rss"
    pad_coils: Optional[int] = None
    use_seed: bool = True

@georgeyiasemis
Copy link
Contributor

Hi @zwep. This in the .yaml file should work:

transforms:
    cropping:
        crop: null

@zwep
Copy link
Author

zwep commented May 10, 2023

Riight of course. So then I also need to incorporate the following changes:

        transforms:
            cropping:
                crop: null
            sensitivity_map_estimation:
                estimate_sensitivity_maps: true
            normalization:
                scaling_key: masked_kspace

@georgeyiasemis
Copy link
Contributor

Actually, I see the problem; for inference, yes, you need to put it like that

Riight of course. So then I also need to incorporate the following changes:

        transforms:
            cropping:
                crop: null
            sensitivity_map_estimation:
                estimate_sensitivity_maps: true
            normalization:
                scaling_key: masked_kspace

@georgeyiasemis
Copy link
Contributor

For training and validation, this should work:

        transforms:
             crop: null
             estimate_sensitivity_maps: true
             scaling_key: masked_kspace

I will open an issue to fix it for inference.

@zwep
Copy link
Author

zwep commented May 10, 2023

Can you also tell me why there is this difference between training and validation, and inference? I thought that the same code would be used to parse these dictionary keys

@georgeyiasemis georgeyiasemis linked a pull request Jun 25, 2023 that will close this issue
jonasteuwen pushed a commit that referenced this issue Jun 28, 2023
…, #247, #249)

Minor code fixes:
* Fix transforms creation for inference using `dict_flatten`

Config fixes:
* Update projects/<configs>  to new transforms config:
``` 
cropping:
    crop: null
sensitivity_map_estimation:
    estimate_sensitivity_maps: true  # Estimate the sensitivity map on the ACS
normalization:
    scaling_key: masked_kspace
```

Documentation fixes:
* Update some documentation files
georgeyiasemis added a commit to georgeyiasemis/direct-ssl that referenced this issue Aug 31, 2023
…I-AI#244, NKI-AI#245, NKI-AI#246, NKI-AI#247, NKI-AI#249) (#3)

Minor code fixes:
* Fix transforms creation for inference using `dict_flatten`

Config fixes:
* Update projects/<configs>  to new transforms config:
``` 
cropping:
    crop: null
sensitivity_map_estimation:
    estimate_sensitivity_maps: true  # Estimate the sensitivity map on the ACS
normalization:
    scaling_key: masked_kspace
```

Documentation fixes:
* Update some documentation files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants