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

ENH: Update deterministic tractography #120

Merged
merged 18 commits into from
Apr 12, 2021

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Apr 3, 2021

Related to #105.This adds an exercise to change stopping criteria to BinaryStoppingCriterion for tracking and a quick visualization exercise to change the color of streamlines. I think this is the simplest alternative for the stopping criteria suggested in #105. ACT may be good to add in the future as a challenging exercise or more advanced lesson.

Will mark ready for review once I figure out the rotations for the cameras so I can add a figure to the solution output.

@kaitj kaitj requested a review from jhlegarreta April 3, 2021 17:09
@netlify
Copy link

netlify bot commented Apr 3, 2021

Deploy preview for carpentries-dmri ready!

Built with commit 18d790f

https://deploy-preview-120--carpentries-dmri.netlify.app

@kaitj kaitj mentioned this pull request Apr 3, 2021
@kaitj kaitj marked this pull request as ready for review April 3, 2021 23:46
@kaitj kaitj requested a review from josephmje April 3, 2021 23:46
@kaitj
Copy link
Collaborator Author

kaitj commented Apr 3, 2021

PR should be ready to review now. I am open to adding an exercise for the probabilistic tractography if we can think of one, but given what has been taught in the current episodes, this may a good enough state for now.

Copy link
Contributor

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Looks a fair exercise taking into account our time constraints.

As for the visualization, I was more thinking in #105 to apply the FA scalar map rather than applying the same color and just changing the opacity to a value of 0.2 (which is IMHO misleading as it is the one used for the FA). But as I said, given our constraints, it is fine the way is set up.

IMHO the notebooks should be modified to include the exercise within this PR, otherwise we'll again run the risk of having out-of-sync Markdown vs. notebooks.

As for the prob tracking episode, I'm fine if we do not add an exercise; I've put a note in the issue with other relatively easy candidates.

_episodes/deterministic_tractography.md Outdated Show resolved Hide resolved
_episodes/deterministic_tractography.md Outdated Show resolved Hide resolved
_episodes/deterministic_tractography.md Outdated Show resolved Hide resolved
@jhlegarreta
Copy link
Contributor

Please consider merging #123, #124 and #125 before, and consider using the get_fdata() call in here and in any other eventual call (e.g. when changing the notebooks as requested above).

@kaitj
Copy link
Collaborator Author

kaitj commented Apr 4, 2021

Going to change this to draft now - planning on sitting down one of the nights next week to just solely update this episode with figures, sync content between markdown and notebook(s) and run a linter through the notebook.

@kaitj kaitj marked this pull request as draft April 4, 2021 20:19
@kaitj kaitj mentioned this pull request Apr 4, 2021
@kaitj kaitj force-pushed the tractography_exercises branch from 43da7e1 to 9b2b6bb Compare April 5, 2021 15:45
@jhlegarreta
Copy link
Contributor

3a3069d is not necessary:
https://carpentries-incubator.github.io/SDC-BIDS-dMRI/local_tractography/index.html

The people behind Markdown made life a little bit easer.

@kaitj
Copy link
Collaborator Author

kaitj commented Apr 5, 2021

3a3069d is not necessary:
https://carpentries-incubator.github.io/SDC-BIDS-dMRI/local_tractography/index.html

The people behind Markdown made life a little bit easer.

Ahh, thats good to know. Will revert the commit.

@kaitj kaitj force-pushed the tractography_exercises branch from 3a3069d to 9b2b6bb Compare April 5, 2021 23:45
@kaitj kaitj marked this pull request as ready for review April 6, 2021 17:53
@kaitj kaitj self-assigned this Apr 6, 2021
@kaitj kaitj added status:in progress Contributor working on issue type:enhancement Propose enhancement to the lesson labels Apr 6, 2021
@kaitj kaitj changed the title ENH: Add exercise to deterministic tractography ENH: Update deterministic tractography Apr 6, 2021
@kaitj
Copy link
Collaborator Author

kaitj commented Apr 6, 2021

PR should be ready to review now. Markdown and notebooks should be synced - I removed the latex call as it doesn't render properly in the markdown - we can add it back in later on as an image if needed.

Exercise now asks the learner to color each point of the streamline with the FA map with addition of having to transform the streamlines from world to native space. I believe these different spaces were covered in the IntroMRI so I didn't include any additional detail aside from the necessary command.

Also used this PR to update all of the deterministic tractography figures (see #131).

@kaitj kaitj requested a review from jhlegarreta April 6, 2021 18:02
Copy link
Contributor

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

I haven't checked the exercises, but fine for merging if you wish.

LaTeX code not rendering might eventually be fixed thanks to this carpentries/carpentries-theme#13, but fine removing until then.

I think the picture added at the end corresponds just to the first exercise, and the picture corresponding to the second one is missing. I would add the picture corresponding to each exercise right after the exercise and before the next exercise if that is possible.

_episodes/deterministic_tractography.md Outdated Show resolved Hide resolved
_episodes/deterministic_tractography.md Outdated Show resolved Hide resolved
_episodes/deterministic_tractography.md Outdated Show resolved Hide resolved
@kaitj
Copy link
Collaborator Author

kaitj commented Apr 8, 2021

Thanks for finding the line breaks! Should be resolved fixed now in the most recent commit, though it was nice to see that it didn't break the rendering (at least in the netlify preview).

As for the image, I didn't include a screenshot for exercise 1 given we did not ask for visualization unless you are talking about something different?

Will merge this end of the week barring any other issues found

@jhlegarreta
Copy link
Contributor

As for the image, I didn't include a screenshot for exercise 1 given we did not ask for visualization unless you are talking about something different?

Can be added in a separate PR, but if we do not visualize we do not have any visual clue of how the tracking performed. Learners might be told to just reuse/copy the visualization lines in the episode to show the result of the tracking.

@kaitj
Copy link
Collaborator Author

kaitj commented Apr 9, 2021

Added tract visualization and fixed spelling mistake to exercise 1 in 18d790f

@kaitj kaitj merged commit 9179bcd into carpentries-incubator:master Apr 12, 2021
@kaitj kaitj deleted the tractography_exercises branch April 12, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:in progress Contributor working on issue type:enhancement Propose enhancement to the lesson
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants