Skip to content

Code/pipeline review #9

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Code/pipeline review #9

wants to merge 1 commit into from

Conversation

mattcieslak
Copy link

I'm adding some comments for discussion

@@ -146,6 +147,7 @@ def process_run(name_source, layout, run_data, out_dir, temp_dir):
concat_ihmt_file = os.path.join(temp_dir, f'concat_{name_base}')
concat_ihmt_img.to_filename(concat_ihmt_file)

# How many volumes are in ihmt?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are five volumes with different contrasts. I took the dwidenoise step from ihmt_proc.

Comment on lines +463 to +466
# There is a final step here that has to happen, which is you average together
# all the transforms and apply the inverse to the template.
# This prevents the template from drifting.
# See https://github.com/PennLINC/qsiprep/blob/c74ff2f041a9ff6121042d30b5467af6082be85f/qsiprep/workflows/dwi/hmc.py#L273
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on this? I think this might be one spot where QSIPrep's approach and ihmt_proc's differ. The template is only recalculated with a voxel-wise average, so I don't think there's any opportunity for it to drift.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It currently won't be an issue here because you're only doing a single iteration. If there were multiple iterations, it's possible that the template brain will move around in the FOV. The workflow in qsiprep is based on the antsMultivariateTemplateConstruction.sh script, where a couple iterations are always used. If there is a lot of motion the first average won't look very good, so the registrations to it will still have some error. Over the multiple iterations the template starts to converge to something nice.

I'm not sure if multiple iterations is overkill here. It would depend on how much motion you expect over these volumes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method in ihmt_proc only runs it once. I think it should be fine, but we can revisit and apply a multi-iteration method if it doesn't work well.

Comment on lines +227 to +228
# What do you think about making a project-wide setting for what to do with calculations that
# involve division? It's an unstable operation and we should handle it consistently.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on this idea?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the problems with T1/T2 "myelin maps" is that if the denominator gets close to zero you get extreme numbers. I was imagining something like a config setting for what to do when there is a division and you see extreme values. For example, if some voxels are inf or extreme outliers, should they be capped? Or set to nan? Or should we leave them as is? It makes sense that if we offer this option for one of the calculations that we should offer it for all of them.

Comment on lines +224 to +225
# Is there an advantage to keeping ihmt_files_t1space as a list?
# It's less intuitive than using variables
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to loop over the images, but there are more intuitive ways like you said. Maybe a dictionary so it's still easier to organize the image types in a single variable I can index?

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

Successfully merging this pull request may close these issues.

2 participants