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

Fixing brainmodel and accommodating non-standard cifti #3

Merged
merged 9 commits into from
Feb 10, 2019

Conversation

mychan24
Copy link
Contributor

@mychan24 mychan24 commented Feb 8, 2019

Issue (1). Using read_cifti() on a cifti file with both surface and volume based structures only reads in the attributes of the surface structures.

  • Edited parse_brain_model.R

Issue (2). dtseries.nii from MSC (Midnight Scanning Club) has non-standard dimension where the dense data is on the first dimension, so when the data is assigned into an array, it is arranged incorrectly.

  • Edited cifti_data.R with comments documenting why. This does not affect cifti with standard dimensions. Two .md vignettes were added to show the error using the original cifti functions and the updated functions. The vignettes also show that the updated script does not affect how standard cifti are loaded in (e.g., data from download_cifti_data)

Given people may use these function with the MSC data, I think its worth incorporating it into the function.

…nmodel info.

* Original script replaces the brain model info for voxel-type structures with only voxel_ijk
* Structure label of voxel_based structure is therefore not extracted by parse_brain_label() (used by get_cifti_type())
…on (e.g., MSC).

* Original code would read in cifti with dense data (vertex) as rows incorrectly.
@coveralls
Copy link

coveralls commented Feb 8, 2019

Coverage Status

Coverage decreased (-0.002%) to 0.163% when pulling 67abc14 on mychan24:master into 11cbcda on muschellij2:master.

@muschellij2
Copy link
Owner

Thanks for the PR. Before accepting please formats the vignettes to Rmd files (see usethis::use_vignette for the header). Also, for my packages, I need them to be fully reproducible (even the vignettes). Therefore, we need to include the cifti in the package or have them uploaded somewhere like figshare.

mychan24 and others added 3 commits February 8, 2019 20:14
* These Rmds are mroe documentation of this specific problem than vignettes meant to accompnay CRAN distribution or How-tos.
@mychan24
Copy link
Contributor Author

mychan24 commented Feb 9, 2019

Hi, I added the Rmd and moved them into a folder call test_Rmd instead of letting them stay in the vignette. As they are not really how-tos or vignettes meant to distribute to CRAN. It was more to document the issue for this particular update, and if it works well, we could eventually remove them.
The Rmd will download the needed data from openneuro if it is not locally available, and is reproducible now on different machines.

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