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

BUG: Remove duplicate probabilistic tractography episode #52

Merged
merged 1 commit into from
Jan 29, 2021
Merged

BUG: Remove duplicate probabilistic tractography episode #52

merged 1 commit into from
Jan 29, 2021

Conversation

jhlegarreta
Copy link
Contributor

Remove duplicate probabilistic tractography episode.

Take advantage of the commit to:

  • Make the episode titles consistent.
  • Fix the figure captions in the probabilistic tractography episode.

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.

Was wondering why there was a duplicate - thanks for deleting!

Being a little bit picky here, but wondering if it might be more accurate to say "Streamlines representing white matter" rather than "White matter streamlines"?

Remove duplicate probabilistic tractography episode.

Take advantage of the commit to:
- Make the episode titles consistent.
- Fix the figure captions in the probabilistic tractography episode.
@jhlegarreta
Copy link
Contributor Author

Was wondering why there was a duplicate - thanks for deleting!

The duplication was probably inadvertently introduced by me (sorry) when re-organizing lessons #47. Its contents were exactly the same as in _episodes/09-probabilistic_tractography.md.

Now, that led me to investigate a little bit more, since I remember a 04-tractography.* file being in place a few months ago.
It is still there as a notebook, with its solutions, but it lacked the corresponding episode Markdown file. I've gone through its contents, and I'd say that it fits into what I named as 08-deterministic_tractography.md. Looks an oversight on my end not to have moved its contents/renamed the file in #47.

So with some renaming to the notebook to make things consistent, some refactoring to (re)move part of the introduction in place/to _episodes/06-tractography.md, and some to _episodes/07-local_tractography.md, I think the lesson would already be in good shape !

A few more thoughts on that:

  • Not sure why some (duplicate/deprecated) notebooks exist in the files directory. Shall we remove them in a separate PR?
  • In order to avoid the need to rename all files every time these sort of things happen, or when a new episode is added (no the figures folder is also inconsistent with the current episode/notebook naming), we may want to adopt a convention as suggested here in a future PR.

Also, in a separate PR we may need to stick to the 100-character line length format recommended by the Carpentries. I have used 80 consistently (at least in the markdown files), so maybe we are good using 80 all over the place. Looks like the Carpentries provide a script to check for this; interesting to be added to the CI in a future PR.

Being a little bit picky here, but wondering if it might be more accurate to say "Streamlines representing white matter" rather than "White matter streamlines"?

Agreed. Done in 0cea529. Force pushed.

@kaitj
Copy link
Collaborator

kaitj commented Jan 25, 2021

Not sure why some (duplicate/deprecated) notebooks exist in the files directory. Shall we remove them in a separate PR?

We definitely should down the line once content is ready for delivery with the new dataset! They were kept in there during the transition as I think they were for the old dataset (ds000030 I believe). I will also have to remember to remove that from the OSF so it is only ds000221.

In order to avoid the need to rename all files every time these sort of things happen, or when a new episode is added (no the figures folder is also inconsistent with the current episode/notebook naming), we may want to adopt a convention as suggested here in a future PR.

Definitely open to it! I don't think there was any particular convention we were following previously.

Also, in a separate PR we may need to stick to the 100-character line length format recommended by the Carpentries. I have used 80 consistently (at least in the markdown files), so maybe we are good using 80 all over the place. Looks like the Carpentries provide a script to check for this; interesting to be added to the CI in a future PR.

80 sounds good to me and yes I think it would be good to add to the CI! Unrelated, it may be good to add an auto-assign for reviewers if it passes the CI.

@jhlegarreta jhlegarreta merged commit b675bbf into carpentries-incubator:master Jan 29, 2021
@jhlegarreta jhlegarreta deleted the RemoveDuplicateProbTractographyEpisode branch January 29, 2021 01:33
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