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: Add csd exercise #117

Merged
merged 19 commits into from
Apr 17, 2021
Merged

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Apr 2, 2021

Resolves Relates to #30 - adds an exercise to simulate crossing angles of 90, 60, 45, 30, and 20 degrees. I did have run into some issues simulating with default_sphere and ended up using repulsion724 in the solutions. If someone can double check that, that would be great. I've also left the exercise out of the jupyter notebooks for now. Also have a figure saved showing the solutions at each crossing angle, however can't seem to get it to load in the solutions. If anyone knows how, the file is named odf_multiple_angles.png in the CSD figure directory.

We can add more exercises later on and also increase the difficulty, but I think this is a good place to start to see where CSD may start to fail.

Another exercise mentioned would be to actually perform the deconvolution, but I think we need to dig a little deeper into this dataset before that. I think as we expand the episode to include more details spherical harmonics and figure out some of the quirks we are seeing with the dataset, we can add an exercise to deconvolve with different harmonic orders.

@kaitj kaitj added the type:enhancement Propose enhancement to the lesson label Apr 2, 2021
@netlify
Copy link

netlify bot commented Apr 2, 2021

Deploy preview for carpentries-dmri ready!

Built with commit 615e43f

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

@netlify
Copy link

netlify bot commented Apr 2, 2021

Deploy preview for carpentries-dmri ready!

Built with commit c68dcfa

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

@kaitj kaitj requested a review from josephmje April 2, 2021 23:50
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.

Have not tried c&p-ing the code into a script/notebook to try.

IMHO: the exercise statement is misleading:

In this episode, the ODF of a single fibre population (i.e. no crossing fibres) was visualized.

I'd say that it is inaccurate; the reconstructions, if visualized with enough detail, would show crossing fibers.

If this was meant to refer to the FRF, then we run the risk of generating another confusion by offering learners an exercise that creates fODFs of crossing fibers, which some may think that could be used as a sort of advanced FRF.

So I'd restate the exercise statement.

the file is named odf_multiple_angles.png in the CSD figure directory.

No figure has been added with the commit. Since the jupyter notebook part is still missing, it can done when adding those.

Marking the PR as a draft might be useful until it is complete.

Also, I wouldn't mark #30 as resolved; in #30 it was suggested (unless the wording used was inaccurate or confusing) to perform the actual deconvolution given the peaks for crossing fibers.

Another exercise mentioned would be to actually perform the deconvolution, but I think we need to dig a little deeper into this dataset before that. I think as we expand the episode to include more details spherical harmonics and figure out some of the quirks we are seeing with the dataset, we can add an exercise to deconvolve with different harmonic orders.

Reading this maybe #30 was unclear: it implied using synthetic data (a voxel) (or voxels from the data containing crossings at different angles might do the job) to simulate a signal containing crossings at different angles. I'll add that to the issue.

But I agree, either solution requires more time to investigate and develop in case of the synthetic signal.

Given the time limitations, we may want to go with what this PR proposes.

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

kaitj commented Apr 3, 2021

I'd say that it is inaccurate; the reconstructions, if visualized with enough detail, would show crossing fibers.

Fair point, I've restated the sentence to note in the episode we visualized the response function's ODF.

No figure has been added with the commit. Since the jupyter notebook part is still missing, it can done when adding those.

You're right - committed, but forgot to push. Figure should be present now.

Also, I wouldn't mark #30 as resolved

Updated the initial PR so it mentions #30 without linking to the issue.

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.

Not adding the changes to the notebooks in this PR increases again the chances of going out-of-sync across the markdowns and notebooks.

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

Please consider merging #123, #124 and #125 before. Thanks.

@kaitj
Copy link
Collaborator Author

kaitj commented Apr 4, 2021

Similar to #120 - going to change this to draft now with plans to sit down one of the nights next week to just solely update this episode with figures, sync content between markdown and notebook(s), run a linter through the notebook and address the changes..

@kaitj kaitj marked this pull request as draft April 4, 2021 20:20
@kaitj
Copy link
Collaborator Author

kaitj commented Apr 17, 2021

Notebooks and markdown should be synced. Added a solutions notebook for the exercise, and removed the skip nbval tags in the non-solutions notebook. Actions workflow has been updated to reflect this. Figures also updated after re-running through notebooks (which resolves #131 by updating the last of 3 notebooks).

@kaitj kaitj marked this pull request as ready for review April 17, 2021 02:27
@kaitj kaitj requested a review from jhlegarreta April 17, 2021 02:27
@kaitj kaitj linked an issue Apr 17, 2021 that may be closed by this pull request
3 tasks
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.

Thanks Jason.

@kaitj kaitj merged commit 4050d12 into carpentries-incubator:master Apr 17, 2021
@kaitj kaitj deleted the add_csd_exercise branch March 2, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Propose enhancement to the lesson
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update figures
3 participants