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

Denoising patch radius clarifications #845

Open
araikes opened this issue Nov 5, 2024 · 3 comments
Open

Denoising patch radius clarifications #845

araikes opened this issue Nov 5, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@araikes
Copy link
Contributor

araikes commented Nov 5, 2024

Summary

In v. 0.22.0 and forward, the behavior of --dwi-denoise-window changed. Previously, not setting this parameter (thus auto) set dwidenoise to run with an extent window of 5x5x5 and patch2self to run with a radius that depended on the number of volumes but for adequately sized datasets (number of non-b0 volumes > 50) the default patch radius = 0x0x0. In v 0.22.0 and forward, dwidenoise approach has not changed but patch2self now gives an error because the variable dwi_denoise_window is no longer set prior to the call (it is only created when using dwidenoise and auto for the window). See #827.

Developer recommendations for dwidenoise and patch2self suggest the following about the extent window/patch radius:

  1. dwidenoise by default will select a patch size that minimally exceeds the number of diffusion weighted image: "By default, the command will select the smallest isotropic patch size that exceeds the number of DW images in the input data, e.g., 5x5x5 for data with <= 125 DWI volumes, 7x7x7 for data with <= 343 DWI volumes, etc." (https://mrtrix.readthedocs.io/en/dev/reference/commands/dwidenoise.html)
  2. From @ShreyasFadnavis in Patch2Self error #827, the recommended patch radius for patch2self is 0x0x0 for human data, though infant and NHP (and certainly small animal) data may better use 1x1x1 or larger.

In light of these two recommendations, the question is: How much control is desired over the patch radius? There are two (probably more) relatively straightforward alternatives:

  1. Allow users to have always and complete control over the patch radius. When not specified, "auto" is fixed to correctly propagate to both dwidenoise and patch2self to allow for automatic selection (dwidenoise) or a default 0x0x0 (patch2self). When specified, it is passed to the relevant denoising function as an isotropic patch size.
  2. Control over patch2self only. Because dwidenoise will choose a sane default value, altering the patch radius may have unintended consequences if too large or too small relative to the number of volumes. However, there may be useful experimentation with the patch radius for patch2self (or in cases of infant/NHP data) where being able to set a non-default value is reasonable and possibly necessary.
  3. A third option would be to allow no control, always allowing dwidenoise to auto-select the extent window and using a 0x0x0 patch radius for human data (and maybe a 1x1x1 for infant data).

Looking for thoughts before working on a solution to the current error where patch2self is not working at all.

@mattcieslak feel free to update or clarify as needed.

@araikes araikes added the enhancement New feature or request label Nov 5, 2024
@chiuhoward
Copy link

chiuhoward commented Nov 22, 2024

Hi all,

came to report this too! I’m working with infant data so I’ll be interested to hear the discussion on 0x0x0 versus 1x1x1.

This is on 1.0.0rc1.

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 117, in build_workflow
    retval["workflow"] = init_qsiprep_wf()
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/base.py", line 83, in init_qsiprep_wf
    single_subject_wf = init_single_subject_wf(subject_id, session_ids)
  File "/opt/conda/envs/qsiprep/lib/python3.10/site-packages/qsiprep/workflows/base.py", line 401, 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 247, 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

@araikes
Copy link
Contributor Author

araikes commented Nov 22, 2024

Hi @chiuhoward,

No fix is in yet but conversation is on-going.

@tsalo
Copy link
Member

tsalo commented Nov 26, 2024

I think we've resolved the conversation on Slack and can translate it into action items for QSIPrep:

  1. Stop setting patch-radius for patch2self since the patch2self devs are dropping that parameter completely.
  2. Infer the appropriate patch radius for dwidenoise with closest_odd(np.floor(np.cbrt(n_volumes))).
    • The main reason to explicitly set it is so we can have the patch radius accurately described in the boilerplate.
  3. Add a warning if users combine --denoise-after-combining with --patch-radius auto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants