Peak extraction isbi

[mdesco]

This is for the ISBI challenge for participant to try peak extraction on their ODF file

[Garyfallidis]

Okay we need to sent the link to the code to the ISBI organizers. It looks good to me. I am going to go ahead and merge it. We can always do a new PR if people find problems with it. GGT!

[matthew-brett]

Can I humbly ask for the policy on merging pull requests?

So here - I'm sure it wasn't possible for some of the others to have a look. I think it's harder to go back over code after it's merged.

So maybe your policy is minor review, merge fast. But there are downsides that that policy, especially near a release. So maybe it would be good to have a discussion and agree policy?

I'm sure you've seen in - say - numpy previous generation - that merging too fast can cause a lot of friction if you are not careful. But in any case it might not suit everyone and it would really help to get the policy straight so that no-one has to worry.

[Garyfallidis]

Yes, I agree. Fast merging is a problem. I completely get that. Unfortunately, we needed to give that script to the organizers quickly but we made some tests here with the isbi data. In one of my previous comments I said that we are on a quick deadline. The code is short and it does not interfere with the release. For most rules there are sometimes exceptions. I think this is the case with this PR. Let me know if you think otherwise and thank you for your suggestion. Additionally, it's not difficult to look at a closed PR and not difficult to create a new branch and a new PR. I look at closed PRs all the time.

[matthew-brett]

Yes, but the reason for the discussion on the mailing list is so that everyone knows what the policy is. That's good because then people don't worry about having a PR merged before they've had a chance to have a look.

In this case - did the organizers really need the code merged into master? Could the not have a tarball from a branch? Well - no need to go back over this one - but still - I for one would like to know what is 'the dipy way'....

[Garyfallidis]

That is a good point. We have not discussed or wrote somewhere what is the policy with the PRs. You are more experienced than me on the topic and you are a dipier too. Please go ahead and propose a solution.

Here are some thoughts.

If someone makes a PR with a new feature and for two weeks he has not had any comment from the team he can go ahead and merge the project.
During release period this should be one week. For a minor PR to be merged at least another person should review the code. For a major PR at least two. In extreme cases someone can ask for permission to merge quicker. If there is no disagreement then he can go ahead.

I think these rules are quite flexible. I am not very keen on pushing very strict rules. I feel it could create more frustation in the long term.

Concerning the challenge we are giving both internal and external scripts. So, not everything was added in dipy.

[arokem]

Hi Eleftherios,

This seems OK to me as a rule-of-thumb: two week window for reviews after
which merges can carefully proceed, if there are no objections. With a
couple of caveats:

On Fri, Mar 1, 2013 at 9:02 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

That is a good point. We have not discussed or wrote somewhere what is the
policy with the PRs. You are more experienced than me on the topic and you
are a dipier too. Please go ahead and propose a solution.

Here are some thoughts.

If someone makes a PR with a new feature and for two weeks he has not had
any comment from the team he can go ahead and merge the project.
During release period this should be one week. For a minor PR to be merged
at least another person should review the code. For a major PR at least
two. In extreme cases someone can ask for permission to merge quicker. If
there is no disagreement then he can go ahead.

I think that during the run-up to a release we need to be more careful
with merging new code, not less careful (which is implied in you above
statement). We are constantly trying out the code in its developmental
stages in our day-to-day usage and that gives us the opportunity to fix
things before a release. Having new code introduced before a release
increases the risk that the release will contain incorrect, or suboptimal
code.

Also - I would be careful of making decisions that appear to be serving a
particular agenda that is narrowly defined - "we need to send the code to
the ISBI organizers" comes pretty close to that (and I second Matthew's
argument: you could have just made a tar-ball from that branch, if there
was strict deadline there). I am of course happy that there is the
opportunity to expose more users and developers to dipy, and I know that
you (@Garyfallidis) are very passionate about promoting the project and
getting more people using it and involved in developing it. And that's
great. But I would be careful with the way things might be perceived by
other people involved. I take being a "natural home for collaboration"
(from our mission statement) to mean that decisions are made by some kind
of consensus, not by fiat. Sometimes that takes more time, which is a small
price to pay for keeping a community of developer-scientists happy and
motivated.

That's just my take on it. Would be happy to hear what others think.

I think these rules are quite flexible. I am not very keen on pushing very
strict rules. I feel it could create more frustation in the long term.

Concerning the challenge we are giving both internal and external scripts.
So, not everything was added in dipy.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/141#issuecomment-14323065
.

[matthew-brett]

That seems reasonable to me - I mean both your (Eleftherios') comments and your (Ariel's) additions.

Eleftherios - would you consider putting something like this as an email to the list for discussion? It might be good also for the other projects.

For nipy and nibabel, I try to follow the rule that I never merge without giving good warning. So I might say 'hey this one is urgent, I want to merge in the next few days', then next day 'unless I hear otherwise I'll merge tomorrow', and then I probably wait another couple of days. That way I hope that anyone who had time will get to have a look and see if it is a problem. If there are still comments and the person making the comments has not signed off explicitly, I wait, at least, I think that's what I do.

[Garyfallidis]

Apart from the policy on the PRs I would also like to hear what people think that I should be expected to do for the project. I am impressed that Ariel thinks that this PR somehow violated our mission. And very sad to hear that some developers are not happy with this. I mostly tried to do this quickly because for me it was a minor thing and there was no reason to spend your time with it. Anyway, this is a difficult/busy week for me mainly because of the trip in Boston. I can start this discussion on the mailing list next week. I hope this is okay. But you can go ahead and start it now.

[stefanv]

@Garyfallidis It doesn't really matter what the PR contains (we all liked this one, in fact)--it's more about building a shared sense of ownership. And, as @arokem mentioned, this is personally important to many of us, given that we base our research on dipy now (and that is a feather in your cap). Therefore, I'd like to have enough time to review code so that we don't have to revisit too many things later. With API re-design, for one, things become even harder (and this PR did introduce a new API of sorts), since once it is in use we cannot go back.

I am happy with the rule that each PR needs at least one review and, unless we're talking about a typo or some other triviality, let it simmer for more than one day at least. I hope we've learnt our lessons from the NumPy days :)

FWIW, I don't think @arokem suggested that the PR itself violates our mission--just that skewing our workflow to suit some specific sub-group, when there are work-arounds available, does.

100% GGT