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.

Sign up for GitHub

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

Jump to bottom

ENH: Add SH order theory #106

Merged
merged 3 commits into from
Mar 28, 2021
Merged

ENH: Add SH order theory #106

merged 3 commits into from
Mar 28, 2021

Conversation

kaitj
Copy link
Collaborator

@kaitj kaitj commented Mar 28, 2021

Related to #28 - not entirely sure what you had in mind here. Added brief mention of how the sh order is related to the angular frequency, but I think as it is, it currently provides the relevant details for an introductory lesson.

@kaitj kaitj requested a review from jhlegarreta March 28, 2021 19:05
@netlify
Copy link

netlify bot commented Mar 28, 2021 •
edited
Loading

Deploy preview for carpentries-dmri ready!

Built with commit 83c6995

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

jhlegarreta
jhlegarreta reviewed Mar 28, 2021
View reviewed changes
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.

A few in-line comments. Not all changes have been reflected in the Jupyter Notebook. Was this intentional?

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

jhlegarreta commented Mar 28, 2021 •
edited
Loading

Related to #28 - not entirely sure what you had in mind here. A

As mentioned in #30, introducing the mathematical expressions related to the SH basis and how the deconvolution relates to it graphically will improve the foundations of the episode in the long run. But this is already a step forward.

@kaitj
Copy link
Collaborator Author

kaitj commented Mar 28, 2021 •
edited
Loading

Agreed - I am going to priotize updating for #30 then and come back to #28 at a later date!

@jhlegarreta jhlegarreta merged commit 8deac30 into carpentries-incubator:master Mar 28, 2021
@kaitj kaitj deleted the update_csd branch April 2, 2021 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhlegarreta jhlegarreta jhlegarreta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kaitj @jhlegarreta