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: Add discussion section about camera setup #77

Merged
merged 1 commit into from
Mar 24, 2021
Merged

DOC: Add discussion section about camera setup #77

merged 1 commit into from
Mar 24, 2021

Conversation

jhlegarreta
Copy link
Contributor

Add discussion section about camera setup.

@jhlegarreta jhlegarreta added the type:discussion Discussion or feedback about the lesson label Mar 20, 2021
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Mar 20, 2021

Following the discussion in #69.

A few notes:

  • The figure is missing. As discussed with @kaitj , we may need to create our own figure. Also, the figure location might need to be rethought (not sure if the fig/{non-episode} location would be valid.
  • ENH: Put the fury camera setup into a function #76 should be merged before this.
  • This should probably be be cross-referenced from the episode and notebook. Not sure about the best/cleanest way to do it.

@netlify
Copy link

netlify bot commented Mar 20, 2021

Deploy preview for carpentries-dmri ready!

Built with commit c83e323

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

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.

This looks good to me. The sagittal view might actually be sagittal left. Was originally suppose to be sagittal right, and I think I never changed the comment. I can try to create a figure later this week to add to this, but agreed we should figure out if the image filepath is valid.

@jhlegarreta
Copy link
Contributor Author

This looks good to me. The sagittal view might actually be sagittal left. Was originally suppose to be sagittal right, and I think I never changed the comment. I can try to create a figure later this week to add to this, but agreed we should figure out if the image filepath is valid.

I'd first merge #76, then do a separate PR to either change the title to sagittal left or generate a sagittal right view. But that is independent of the additions of this PR. I had similar conocerns about the coronal view being posterior (but did not had a detailed look at it): both left/right and anterior/posterior changes can be done in a separate PR if appropriate.

OK as for generating the figure as time permits.

@kaitj
Copy link
Collaborator

kaitj commented Mar 22, 2021

I had similar conocerns about the coronal view being posterior (but did not had a detailed look at it)

Yeah, I realized I never had a chance to double check orientations. I'll put it on my to do list!

@kaitj
Copy link
Collaborator

kaitj commented Mar 22, 2021

Leaving this here more as a note for now so its not forgotten / can be double checked.
Looks like two views that need to be changed are:

  • Sagittal right -> Sagittal left
  • Coronal anterior -> Coronal posterior

Add discussion section about camera setup.
@jhlegarreta jhlegarreta merged commit 88031be into carpentries-incubator:master Mar 24, 2021
@jhlegarreta jhlegarreta deleted the AddDiscussionAboutCameraSetup branch March 24, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:discussion Discussion or feedback about the lesson
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants