-
Notifications
You must be signed in to change notification settings - Fork 437
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
Peak extraction isbi #141
Peak extraction isbi #141
Conversation
STY: PEP8, white-space, unused import.
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! |
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. |
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. |
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'.... |
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. 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. |
Hi Eleftherios, This seems OK to me as a rule-of-thumb: two week window for reviews after On Fri, Mar 1, 2013 at 9:02 PM, Eleftherios Garyfallidis <
Also - I would be careful of making decisions that appear to be serving a That's just my take on it. Would be happy to hear what others think.
|
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. |
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. |
@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 |
This is for the ISBI challenge for participant to try peak extraction on their ODF file