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

MAINT: Add a base of CONTRIBUTING guidelines #14

Merged
merged 5 commits into from
Sep 27, 2019
Merged

MAINT: Add a base of CONTRIBUTING guidelines #14

merged 5 commits into from
Sep 27, 2019

Conversation

oesteban
Copy link
Member

To be discussed in tomorrow's telecon (Close #12)

There are some descriptions I'll be working on today and tomorrow
morning before finishing the proposal.

This is related (sideways) to #7 too.

Copy link
Collaborator

@arokem arokem 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! Thanks for bringing in all the knowledge from your previous experience with fmriprep. I agree that scope should be limited, but as indicated by my questions, I am not 100% sure that we are ready to limit it just to the things you mentioned here. Should we take that as a separate discussion, after merging this here?

CONTRIBUTING.md Outdated Show resolved Hide resolved
@richford
Copy link
Collaborator

LGTM. The only minor change I'd recommend is the inclusion of the "imposter syndrome disclaimer" from the nipy/dmriprep repo. I think the extra language can help to further foster inclusion. If the disclaimer is contentious, I'm happy to move that to another issue so that we can have that discussion without holding up this PR.

I agree with the "recognizing contributions" section and I tend to lean more towards maximal authorship inclusion (though my physics bias might be showing here).

@oesteban oesteban force-pushed the master branch 2 times, most recently from c4f0cf4 to 662f586 Compare September 26, 2019 18:28
To be discussed in tomorrow's telecon (Close #12)

There are some descriptions I'll be working on today and tomorrow
morning before finishing the proposal.

This is related (sideways) to #7 too.
@oesteban oesteban marked this pull request as ready for review September 26, 2019 19:36
@oesteban
Copy link
Member Author

Would love to hear opinions from @josephmje and @dPys

@oesteban oesteban requested a review from arokem September 26, 2019 19:40
Copy link
Collaborator

@richford richford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I really appreciate you lending your experience on fmriprep to help make dmriprep a success!

@richford richford mentioned this pull request Sep 26, 2019
@josephmje
Copy link
Collaborator

josephmje commented Sep 26, 2019

Apologies for the late reply. I think this looks fantastic! I saw the contributing guide from tedana and noticed that this is inspired by the same place. Some additional thoughts:

  1. There was talk about running black and potentially a developer setup? Is that still the case? If so, that should be mentioned here.
  2. Including a blurb on the Style Guide and writing in ReStructuredText for the docstrings.

@dPys
Copy link
Collaborator

dPys commented Sep 26, 2019

I agree with the contributing guidelines as outlined. Some notes:
*The numbering of principles seem to all be labeled as '1.' -- I'm guessing this is a typo?
*Should we mention 'parsimony' as a general principle? i.e. if a purely-pythonic tool is available to replace a third-party equivalent, all else being equal, then pure-python is preferred to minimize external dependencies. e.g. preference for dipy's gibbs suppression over and above mrtrix's mrdegibbs which does the equivalent but demands mrtrix be added as a dependency. Now if it were the case that mrtrix was already a dependency, and mrdegibbs was a faster option or consumed less memory resources, then that would require different consideration. But I think the overarching principle would be that parsimony, whenever possible, is preferred. And that wouldn't just be a dmriprep principle, since it is fundamentally a pythonic one.

Curious to hear thoughts on this,
@dPys

@oesteban
Copy link
Member Author

*The numbering of principles seem to all be labeled as '1.' -- I'm guessing this is a typo?

Nope, that's the formatting recommendation of markdown - it will reassign numbers when rendering.

*Should we mention 'parsimony' as a general principle? i.e. if a purely-pythonic tool is available to replace a third-party equivalent, all else being equal, then pure-python is preferred to minimize external dependencies. e.g. preference for dipy's gibbs suppression over and above mrtrix's mrdegibbs which does the equivalent but demands mrtrix be added as a dependency. Now if it were the case that mrtrix was already a dependency, and mrdegibbs was a faster option or consumed less memory resources, then that would require different consideration. But I think the overarching principle would be that parsimony, whenever possible, is preferred. And that wouldn't just be a dmriprep principle, since it is fundamentally a pythonic one.

I agree with this principle in general (not just for dMRIPrep), so for me there is no need to have it in writing. I'm happy including some of the language you proposed if others think we have to make this explicit.

@oesteban oesteban merged commit ae9a467 into nipreps:master Sep 27, 2019
@oesteban oesteban deleted the maint/contributing branch September 27, 2019 17:37
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.

Telecon, September 24th, 1 PM PST
6 participants