Skip to content

Remove names_308_style from BrainNetworksInPython#75

Merged
Islast merged 5 commits intoWhitakerLab:devfrom
Islast:dev
Aug 14, 2018
Merged

Remove names_308_style from BrainNetworksInPython#75
Islast merged 5 commits intoWhitakerLab:devfrom
Islast:dev

Conversation

@Islast
Copy link
Copy Markdown
Collaborator

@Islast Islast commented Aug 13, 2018

  • I'm ready to merge
  • What's the context for this pull request?
    (this is a good place to reference any issues that this PR addresses)
    after a chat with Kirstie we've decided to remove names_308_style since it really isn't in general use.

  • What's new?
    All references to names 308_style are now gone. I've run the test suite and all failures are preexisting and understood so I think it's safe to merge
    I've also thrown in some small changes to the tutorial

  • What should a reviewer feedback on?
    check that 308 is the right number of rows for the residual df to have 🌺

  • Does anything need to be updated after merge?
    (e.g the wiki or the WhitakerLab website)
    The documentation with the docstrings has been changed. It the other documentation will be overhauled anyway. We will need to change the wiki.

@KirstieJane
Copy link
Copy Markdown
Member

Are you also checking the number of regions in the tutorial etc? The whitaker vertes data should have 308 regions read in 😄

There will be something somewhere that says drop the first 41 names. The way we've saved out the regional data, the names file used to have the subcortical measures at the start of the file so I had a line in the code that removed them. This is the action that we want to remove. The user can sort out their own names file!!

@Islast
Copy link
Copy Markdown
Collaborator Author

Islast commented Aug 13, 2018

@KirstieJane I have removed all the names_308_style methods from the code (name stripping, assigning hemispheres, stripping thickness from the measures file etc). I have had to make some adjustments to the NSPN data. The residuals dataframe now has 308 columns as desired (i think?)

@KirstieJane
Copy link
Copy Markdown
Member

KirstieJane commented Aug 13, 2018

That sounds great - removing "_thickness" from the MRI data is something I've wanted to do for ages 🤦‍♀️ Great to have it done here (next place is to update NSPN_rois_behavmerge.py so it doesn't happen in the first place!) I've made an issue: KirstieJane/UCHANGE_ProcessingPipeline#5

@Islast Islast merged commit e713121 into WhitakerLab:dev Aug 14, 2018
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