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

DOC: Update introduction and preprocessing lessons #222

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

Bradley-Karat
Copy link
Contributor

@Bradley-Karat Bradley-Karat commented Apr 6, 2023

Added in and expanded upon the current information in the introduction and preprocessing lessons. This includes changes to improve clarity and the logical flow of the lesson. Also changed the first figure of the introduction lesson to something that is a bit more clear (i.e. easier to understand the signal change via gradient direction change).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@kaitj kaitj left a comment

Choose a reason for hiding this comment

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

Content-wise, this seems good to me.
Should run this through a linter for formatting / consistency with other lessons

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.

The old image should be removed from the repository if it is no longer used.

Do not forget to apply them to the Jupyter Notebook.

Also, please squash all commits into a single one in this case.

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

@Bradley-Karat would you have time to address the comments or do you prefer to hand it over to somebody else? Thanks.

1 similar comment
@jhlegarreta
Copy link
Contributor

@Bradley-Karat would you have time to address the comments or do you prefer to hand it over to somebody else? Thanks.

@Bradley-Karat
Copy link
Contributor Author

@Bradley-Karat would you have time to address the comments or do you prefer to hand it over to somebody else? Thanks.

Sorry Jon just saw this now. I can go through and address some of these comments.

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 for the effort Bradley. A few notes:

  • Can you please add a commit prefix: e.g. DOC to signal that this his a documentation commit? Please see the commit history for guidance on this.
  • We have tried to limit the line length to 79 characters so that reading the raw files is easier and for the sake of consistency. Applies to both the Markdown and Jupyter Notebooks. Although the Markdown and Jupyter Notebook rendering does not get affected by the line length, keeping a moderate and consistent number helps a lot when reviewing.

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

Also, change the commit subject and message: the preprocessing episode was also changed.

@Bradley-Karat Bradley-Karat changed the title Update introduction to diffusion MRI lesson DOC: Update introduction and preprocessing lessons Aug 23, 2023
@jhlegarreta
Copy link
Contributor

@Bradley-Karat can you please address the comments made?

@netlify
Copy link

netlify bot commented Sep 11, 2023

Deploy Preview for carpentries-dmri failed.

Name Link
🔨 Latest commit db4baa2
🔍 Latest deploy log https://app.netlify.com/sites/carpentries-dmri/deploys/64ff653e416be60008ad9cac

code/introduction.ipynb Outdated Show resolved Hide resolved
_episodes/introduction.md Outdated Show resolved Hide resolved
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.

Please, add a commit prefix (i.e. DOC:) and message body to the commit explaining the changes. This is an excellent post on why this matters.

@jhlegarreta
Copy link
Contributor

@Bradley-Karat Can the comments above be addressed, please?

@jhlegarreta
Copy link
Contributor

Pinging @Bradley-Karat.

Added in and expanded upon the current information in the introduction
and preprocessing lessons. This includes changes to improve clarity and
the logical flow of the lesson. Also changed the first figure of the
introduction lesson to something that is a bit more clear (i.e. easier
to understand the signal change via gradient direction change).
Copy link

netlify bot commented Nov 9, 2023

Deploy Preview for sdc-bids-dmri failed.

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/sdc-bids-dmri/deploys/6551230b9eb8613213a65c7d

@Bradley-Karat
Copy link
Contributor Author

@Bradley-Karat Can the comments above be addressed, please?

Sorry Jon just got to this, just made those changes.

@jhlegarreta
Copy link
Contributor

Thanks for addressing the comments @Bradley-Karat.

Usual tests timing out. Merging.

@jhlegarreta jhlegarreta merged commit bfa5595 into carpentries-incubator:main Nov 12, 2023
5 of 9 checks passed
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.

3 participants