MDR

[JSousa-UoL]

This PR will close issue #164.

This draft PR doesn't contain docstrings (some place holders).

The purpose is to discuss code structure and think of smarter ways to do unit testing. Please ignore PEP8 and other minor issues for now, I'd like to hear higher level feedback at this stage.

[pep8speaks]

Hello @JSousa-UoL, thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2022-04-20 12:21:15 UTC

[alexdaniel654]

I haven't gone through with a fine tooth comb and done line by line comments (apart from the bits I noticed when testing) but rather just reviewed the overall structure so far.

Notebooks

• I think it would be better to just have a single MDR notebook, the part you're demonstrating is the motion correction, the fact its being used for different measurements is incidental. You could still keep both a T1 and diffusion example, just in a single motion_correction notebook.
• Might be nice to include difference images to highlight the effect moco has had.

MOCO Module

• General structure seems fine. Obviously documentation is lacking at the moment.
• It was really handy that in the T1 mapping moco notebook, the fitted and coregistered images were output with time along dimension three so you could quickly see the effect moco was having on each slice (example below). It might just be that it works well for my workflow, do you think it would be useful to have a flag in the to_nifti method that swaps dimensions 3 and 4 so you can see slices through time easily on all data rather than just 2D data? Might also be worth adding a similar figure to the notebook.
  
• Why is there no get_log method?
• If you're expecting people to retrieve things like fitted with getters then self.mdr_results etc should be the python version of private variables i.e. self._mdr_results

Tests

• I understand why you've only done tests for diffusion. Just thinking of ways to remedy this without making the test runtime silly, how does the runtime of the MDR package scale with a. image size and b. how much motion there is in the image and c. the number of time points/b-vals its registering? All these could perhaps be controlled a bit more by using simulated data rather than real data. I started working on an in-silico phantom module for nephtools at one point. I'm not necessarily suggesting we add this to ukat, but I could generate some low resolution simulated diffusion and T1 data that's corrupted by a defined amount of motion and save that as a nifti. That way a. the tests would run quicker, b. you could test both DWI and T1 moco and c. we would know what the gold standard, uncorrupted, data should have been like. Just an idea.
• I think the registration.run() step should be in the setup (start of the class) rather than a function within it as most other tests rely on the results of this.
• I assume theres still some work to do on the actual tests as theres still a lot of assert 1==1 and expected values that are empty lists.

[JSousa-UoL]

Thank you so much for your feedback @alexdaniel654. I will take into account your comments about the Notebooks and Tests.

Regarding the MOCO Module section:

• I agree with making self.mdr_results as a private variable.
• There is no get_log method because the flag log=True/False already prints the Elastix log in the terminal and saves it to a .log file. I can disable the terminal printing if you think it's suitable. In your opinion, what should this get_log return? Should it return a string with the entire logging or merge all .log files (creates one per image) into a single file?
• I think we should keep the to_nifti as it is, no need to add new flags. I think the way you visualise the images depends on the software viewer you use. For eg., ITK-SNAP is very dedicated to 3D visualisation, but 4D makes it frustrating to have to choose each timepoint individualy. ImageJ opens these NIfTI images with 2 scrollbars - 1 for "z" and one for "t", so you can freeze one slice or one time point and see what happens in the other dimension (other scrollbar).

[codecov]

Codecov Report

Attention: Patch coverage is 95.22901% with 25 lines in your changes missing coverage. Please review.

Project coverage is 97.57%. Comparing base (b615fd3) to head (58a13da).
Report is 227 commits behind head on dev.

Files	Patch %	Lines
ukat/moco/fitting_functions.py	84.14%	13 Missing ⚠️
ukat/moco/elastix_parameters.py	92.63%	7 Missing ⚠️
ukat/moco/mdr.py	97.86%	4 Missing ⚠️
ukat/moco/tests/test_mdr.py	99.36%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #183      +/-   ##
==========================================
- Coverage   97.92%   97.57%   -0.36%     
==========================================
  Files          39       45       +6     
  Lines        3513     4036     +523     
==========================================
+ Hits         3440     3938     +498     
- Misses         73       98      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.