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: Fix introduction episode non-solution notebook #166

Merged
merged 1 commit into from
Jul 19, 2021
Merged

BUG: Fix introduction episode non-solution notebook #166

merged 1 commit into from
Jul 19, 2021

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Jul 9, 2021

Fix introduction episode non-solution notebook: use the correct dataset
and subject for the analyses.

Fixes:

ValueError: BIDS root does not exist: /home/runner/work/SDC-BIDS-dMRI/SDC-BIDS-dMRI/data/ds000030

raised in
https://github.com/carpentries-incubator/SDC-BIDS-dMRI/runs/3025093598?check_suite_focus=true#step:7:44

Synchronize the markdown episode contents and the Jupyter notebooks
(both the non-solution and solution).

Take advantage of the commit to:

  • Fix the static image relative path.
  • Fix the dataset tree: change the duplicate fmap folder for the
    missing func folder.
  • Add the bids initial dot extension config statement to avoid the
    related warning being displayed.
  • Remove the dipy and fury installation steps: both tools are
    assumed to have been installed during setup.
  • Use the same DWI volume for to be displayed.
  • Fix the output of the bvecs.
  • Removed the unexplained b0 volume slicing block.
  • Add exercise to notebooks.
  • Fix the suffix syntax in exercise.
  • Remove the second exercise: it is already explained/done at the
    beginning of the episode.
  • Fix typos.
  • Remove outputs from non-solution notebook.

@jhlegarreta jhlegarreta requested review from kaitj and josephmje July 9, 2021 21:36
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jhlegarreta
Copy link
Contributor Author

Partially fixes #14 (addresses the third task).

I ignore if the osf copy of the data is a faithful mirror of the openneuro data; i.e. if the treee would look as it is written. I sticked to what I saw in openneuro. Please check and amend the commit as necessary.

I'd say that I'd be better to merge this prior to #165, since the latter's changes can more easily be applied if the rebasing derives in hard-to-solve conflicts.

@netlify
Copy link

netlify bot commented Jul 9, 2021

✔️ Deploy Preview for carpentries-dmri ready!

🔨 Explore the source changes: 3434b27

🔍 Inspect the deploy log: https://app.netlify.com/sites/carpentries-dmri/deploys/60e8de926cd0c000073dad59

😎 Browse the preview: https://deploy-preview-166--carpentries-dmri.netlify.app

Fix introduction episode non-solution notebook: use the correct dataset
and subject for the analyses.

Fixes:
```
ValueError: BIDS root does not exist: /home/runner/work/SDC-BIDS-dMRI/SDC-BIDS-dMRI/data/ds000030
```

raised in
https://github.com/carpentries-incubator/SDC-BIDS-dMRI/runs/3025093598?check_suite_focus=true#step:7:44

Synchronize the markdown episode contents and the Jupyter notebooks
(both the non-solution and solution).

Take advantage of the commit to:
- Fix the static image relative path.
- Fix the dataset tree: change the duplicate `fmap` folder for the
missing `func` folder.
- Add the `bids` initial dot extension config statement to avoid the
related warning being displayed.
- Remove the `dipy` and `fury` installation steps: both tools are
assumed to have been installed during setup.
- Use the same DWI volume for to be displayed.
- Fix the output of the bvecs.
- Removed the unexplained `b0` volume slicing block.
- Add exercise to notebooks.
- Fix the suffix syntax in exercise.
- Remove the second exercise: it is already explained/done at the
beginning of the episode.
- Fix typos.
- Remove outputs from non-solution notebook.
@kaitj
Copy link
Collaborator

kaitj commented Jul 15, 2021

Looks fine to me, I don't recall if the subject is still included in ds000221, but the structure would still be the same (just different subject id). This seems like a good intermediate step - if i recall @josephmje was working on a revamped intro (or was that already merged in)?

@kaitj kaitj mentioned this pull request Jul 15, 2021
_episodes/introduction.md Show resolved Hide resolved
@jhlegarreta
Copy link
Contributor Author

Further improvements can come in separate PRs. This would be a step towards making the episode and its notebooks consistent, and hopefully have the tests passing, since the non-solution notebook is non-functional as it is now.

@jhlegarreta jhlegarreta merged commit 99d17dd into carpentries-incubator:main Jul 19, 2021
@jhlegarreta jhlegarreta deleted the FixAndSyncIntroductionEpisode branch July 19, 2021 12:39
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