search

PennLINC / qsiprep

Preprocessing of diffusion MRI
http://qsiprep.readthedocs.io
BSD 3-Clause "New" or "Revised" License
141 stars 57 forks source link

Patch2Self error #827

Open araikes opened 2 weeks ago

araikes commented 2 weeks ago

Using v 0.22.1, I went to use patch2self as the denoising method (which I've done for every QSIPrep version since 0.11) and got this:

Traceback (most recent call last):
  File "/opt/conda/envs/qsiprep/lib/python3.10/multiprocessing/process.py", line 314, in _bootstrap
    self.run()
  File "/opt/conda/envs/qsiprep/lib/python3.10/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/cli/workflow.py", line 160, in build_workflow
    retval["workflow"] = workflow_builder()
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/base.py", line 82, in init_qsiprep_wf
    single_subject_wf = init_single_subject_wf(subject_id)
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/base.py", line 352, in init_single_subject_wf
    dwi_preproc_wf = init_dwi_preproc_wf(
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/dwi/base.py", line 246, in init_dwi_preproc_wf
    pre_hmc_wf = init_dwi_pre_hmc_wf(
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/dwi/pre_hmc.py", line 238, in init_dwi_pre_hmc_wf
    merge_dwis = init_merge_and_denoise_wf(
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/dwi/merge.py", line 191, in init_merge_and_denoise_wf
    init_dwi_denoising_wf(
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/dwi/merge.py", line 498, in init_dwi_denoising_wf
    Patch2Self(patch_radius=dwi_denoise_window),
UnboundLocalError: local variable 'dwi_denoise_window' referenced before assignment

Looks like the issue is here, where dwi_denoise_window gets defined when config.workflow.dwi_denoise_window == "auto") fordwidenoisebut not forpatch2self`. Based on : https://github.com/PennLINC/qsiprep/blob/2967323a2e9634c18afa7a2b7e76048bed13a025/qsiprep/workflows/dwi/merge.py#L414-L418

May need to either define dwi_denoise_window for the patch2self or pass config.workflow.dwi_denoise_window to the `Patch2Self`` node on L499

araikes commented 4 days ago

@mattcieslak,

I can probably get this fixed if you let me know what the preferred approach for setting the window to go patch2self is.

mattcieslak commented 4 days ago

@ShreyasFadnavis Do you have an algorithm for picking a window size for patch2self?

ShreyasFadnavis commented 3 days ago

Hi @mattcieslak - yes! I do have it in my new paper: https://openaccess.thecvf.com/content/CVPR2024/papers/Fadnavis_Patch2Self2_Self-supervised_Denoising_on_Coresets_via_Matrix_Sketching_CVPR_2024_paper.pdf

The new version of P2S will be available in DIPY soon, but till then a patch size of 0 should be good for all human brain data. If it's non-human brain data, going to a patch of [1, 1, 1] or higher is recommended. I'd just default to 0 in general.

araikes commented 3 days ago

May be worth it then to just hard-code that for now and restrict the dwi_denoise_window parameter for use only with dwidenoise (to avoid people incidentally setting the window to 5 and getting rather poor patch2self results).