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

labelconvert LUTs for Schaefer parcellations #3043

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Lestropie
Copy link
Member

Supersedes #2669.

Includes a portion of #2330.


So there's a few things to unpack here:

  • edited documentation 5ttgen hsvs -white_stem #2330 includes some additional LUTs, however they are interspersed with many other proposed changes. I have manually extracted specifically the new LUTs from that PR.

  • One issue with the contribution of such text files to the code base is that the number of line modifications can be disproportionate with respect to the volume of effort involved. This can unjustly undermine the volume of effort invested by others into modification of source code. One way that we have tried to mitigate this (admittedly with numerous failures to do so in the history prior to coming up with the idea) is by taking those changes that are primarily the result of automatic content generation / for which the line change count is disproportionately high with respect to effort invested, and attributing the corresponding commit to "MRtrixBot [email protected]".
    I have used this in the context of this PR in two ways, which I hope is considered fair to all parties involved.

    • In edited documentation 5ttgen hsvs -white_stem #2330, I have attributed contribution of the novel ordered LUT to @nicdc for having invested the effort in creating a commit and PR. Whereas the original unmodified LUT, which originates from elsewhere and is explicitly attributed to another creator, I have attributed to MRtrixBot.
    • In Schaefer2018 7/17 Networks LUT files #2669, while there was undoubtedly effort involved in creation of these tables, they could have reasonably easily been generated from external LUT data using a relatively small amount of code; and whether this was done manually or using code, I do not think it is equivalent to >10,000 lines of code. So what I have done is attribute the two 200 parcel tables, being equivalent in size to the contribution from @nicdc, to @ppruc, as a total of ~500 lines; the other tables I have attributed to MRtrixBot.
  • There are two different versions of LUTs for the 200-node parcellation. The one from @nicdc in edited documentation 5ttgen hsvs -white_stem #2330 only contains the cortical parcels. Whereas all tables from Schaefer2018 7/17 Networks LUT files #2669 by @ppruc include the FreeSurfer sub-cortical GM parcel labels. Longer-term, I would prefer to have the ability to treat cortical parcellations and sub-cortical parcellations independently and aggregate them as one chooses; just as we do in this published work, and described in ENH labelconvert: Multiple inputs #2481. FOr here I'm content to simply add all files as-is; the software is in no way hard-coded to use these files, they are merely provided in case of utility.

  • While not "bug fixes", given that these changes are purely addition of text files that are not referenced in source code in any way, I'm content to push to master rather than deferring to dev.

@Lestropie Lestropie requested a review from a team November 24, 2024 00:22
@Lestropie Lestropie self-assigned this Nov 24, 2024
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.

4 participants