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

[FIX] Fix typos and formatting #404

Merged
merged 4 commits into from
Feb 26, 2020
Merged

[FIX] Fix typos and formatting #404

merged 4 commits into from
Feb 26, 2020

Conversation

alexandreroutier
Copy link

Hello,

I found very small typos/formatting issues. Also, Gray/Grey names were both present. I used US convention to harmonize this.

Best,
Alexandre

@effigies
Copy link
Collaborator

effigies commented Feb 3, 2020

Suggestions for diffusion should be submitted to https://github.com/bids-standard/bids-bep016.

Suggestions for common-derivatives should be made against the common-derivatives branch. Or just as review suggestions on #265.


Regarding "grey" vs "gray". We don't use either in the current standard, so there's no local precedent.

Gray is consistent with NIfTI-1 and GIFTI. CIFTI-2 uses "gray" in text and "grey" in the constants CIFTI_STRUCTURE_ALL_GREY_MATTER, CIFTI_STRUCTURE_OTHER_GREY_MATTER, so it's not very useful as precedent.

I'm okay with moving fully to gray.

@sappelhoff
Copy link
Member

Thanks for the PR. I am also +1 on adopting US spelling convention, because it would be keeping the current convention in the spec.

We use:

  • behavior instead of behaviour
  • color instead of colour
  • analyze instead of analyse

Etc.

so gray isntead of grey makes sense to me.

@sappelhoff
Copy link
Member

@effigies could we just change the target of this PR to common-derivatives and merge?

Or should @alexandreroutier first revert his changes to src/05-derivatives/06-diffusion-derivatives.md?

@alexandreroutier if you have some time and are faster than @effigies to respond, you could also adhere to his advice and simply make:

  1. a PR to https://github.com/bids-standard/bids-bep016 with your changes in src/05-derivatives/06-diffusion-derivatives.md
  2. a set of code suggestions to this PR: [ENH] BEP 003: Common Derivatives #265 (i.e., all other points you identified outside of src/05-derivatives/06-diffusion-derivatives.md)

@effigies
Copy link
Collaborator

Sorry about the delay... Because some of the files are not in common-derivatives, simply changing the target branch won't work.

@effigies
Copy link
Collaborator

That said, I can just change to "gray" for common-derivatives, merge it into derivatives, and then this PR can be updated.

@sappelhoff
Copy link
Member

cool. @alexandreroutier for simplicity, please revert changes to src/05-derivatives/06-diffusion-derivatives.md here. Then we can merge this.

Afterwards, you are most welcome to make a PR with your changes from src/05-derivatives/06-diffusion-derivatives.md to https://github.com/bids-standard/bids-bep016

@effigies
Copy link
Collaborator

I updated derivatives and then merged in here. I think this is ready for review.

@sappelhoff
Copy link
Member

Thanks @alexandreroutier

@sappelhoff sappelhoff merged commit 627180c into bids-standard:derivatives Feb 26, 2020
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