Hello I wonder if the scope of optimistic merging can be broader. Let's say we have a working implementation of an algorithm which is not the best for one or more of the following reasons.
- Suboptimal data structure choice (linear instead of logarithmic complexity) which results in a minor speed delay during invocation. - Not having type checks or conversions. - Duplicate code which might be refactored out. - Lack of or no documentation But apart from this the PR does what the algorithm it implements correctly. Can we merge such a PR in master and keep it as a private unexposed function while raising an issue to address it's shortcomings ? The function can de decorated to exclude it from the main docs and maybe raise a warning when invoked. I believe the flaws I mentioned can be taken up by someone else in the community. Like a user might contribute documentation or a newcomer might do the refactoring. Then later down the line core Devs can agree and make the function public. It really sucks when there is a working patch for a feature you might want to use but you cannot use it because the PR isn't merged. For example, I don't care if a function takes more time to execute if all I want to do is use it to generate a few statistics or figures while writing a publication. Thanks Vighnesh On 13 Apr 2017 3:17 am, "Juan Nunez-Iglesias" <jni.s...@gmail.com> wrote: > Hi Stéfan, > > I think figuring this out is certainly a high priority when it comes to > being contributor-friendly. > > One thing I don’t know what to do is figure out whether a contributor has > turned on “collaboration”. What am I missing? I can see when *I’ve* turned > it on, but for other pull requests I can’t get any info. Is the only way to > know to try to push? > > A related point of failure that I’ve see is where we have an API > discussion on the PR, and the core members have a back and forth over it, > with confusion about the final outcome for the PR author. I’m still > traumatised by this comment > <https://github.com/scikit-image/scikit-image/pull/953#issuecomment-43598826>. > And the current PRs on manual segmentation > <https://github.com/scikit-image/scikit-image/pull/2584> and nD rescaling > <https://github.com/scikit-image/scikit-image/pull/1522> risk the same > fate. Stéfan and I briefly discussed this off-list and we came up with the > following solution (which we now request comments on): > > - new functions of dubious API go to skimage.future. That’s what the > package was created for. *However*, RAG has languished there for several > releases, so this is not a long-term solution. So, in addition: > - make a note in the TODO.txt for two releases down (would be v0.15 > currently), forcing an API discussion and a move out of skimage.future for > that release. > - For existing functions that require an API change, merge a “good enough” > API, raise an issue on GitHub for the API discussion, and tag it with the > next release. > > For rebasing, if there is nothing left to do in the PR but the rebase, I > suggest that we merge the PR manually in the command line. > > Incidentally, regarding optimistic merging: I do think there is a place > for it in scikit-image, namely, for any PRs that pass tests and coverage > checks, but need stylistic updates. Frankly, it is faster to do these > oneself than to write them in the PR, *and* they inevitably feel petty to > both the reviewer and the contributor. Therefore, I might suggest that we > do perform optimistic merging when there are minor** style issues, and then > fix those issues ourselves. However, if you merge in this situation, you > *must* submit the fix immediately. Otherwise it’s certain to be lost. > > **: by minor, I mean not something like "class Filter: …” "class > Gaussian(Filter): …” =P > > So, to summarise, my proposal when dealing with non-core contributors: > > - avoid massive API discussions. Settle on the simplest solution and add a > to-do. If it’s new functionality, put it in skimage.future. > - avoid all kinds of style nitpicking — it’s faster and easier to fix > these things ourselves. I suggest even doing optimistic merging. > - don’t ask users to rebase unless it’s clear that they are git experts. > Follow github’s advice and merge on the command line. > > Comments are very welcome! > > Juan. > > On 7 Apr 2017, 10:32 AM +1000, Stefan van der Walt <stef...@berkeley.edu>, > wrote: > > Hi everyone, and especially devs > > This afternoon, Nelle Varoquaux gave a short presentation at BIDS on > optimistic merging, a workflow suggested by the late Pieter Hintjens. > In optimistic merging, all PRs are merged into a project, and fixes > happen via additional patches or reverts. While, in my mind, this is > not a viable model for scikit-image, it made me think of our PR review > process. > > A failure mode I observe often is the following: > > - user submits patch > - review happens, often either a) nitpicking on minor things or b) > asking for technically difficult tasks to be performed (rebasing, can be > tricky) > > This introduces several problems, including discouraging new > contributors, and wasting a lot of time (many PRs are left hanging for > months, while waiting for a one-line fix from the original author). > > I'd like to request the following: > > - Ask contributors to open their branches for collaboration (this is on > by default already, probably). > - While reviewing a PR, pull down the branch, look at the diff, and fix > any trivial errors you see. Push the results back up or, if > collaboration is switched off, make a PR against theirs. > - Keep review comments to bigger picture changes, and avoid getting > bogged down in trivial fixes and stylistic changes. > > Let's keep it collaborative and friendly, grow our team of contributors, > and get going on 0.14 :) > > Thanks! > Stéfan > > > > P.S. A question for the git experts: > > I've been looking for ways of making the above process easier. I found > that I can easily download pull requests: > > - In my ~/.gitconfig, I add the following alias (replace 'upstream' > with whatever you normally call the scikit-image main repository): > > [alias] > pullpr = "!f() { git fetch upstream pull/$1/head:pr$1 ;}; f" > > I can now grab a PR with: > > git pullpr 2447 > > and it will appear in the branch pr2447. > > Unfortunately, I don't have a similarly easy way of pushing back, other > than adding the contributor's remote, and pushing back to the PR branch. > Any ideas? > > _______________________________________________ > scikit-image mailing list > scikit-image@python.org > https://mail.python.org/mailman/listinfo/scikit-image > > > _______________________________________________ > scikit-image mailing list > scikit-image@python.org > https://mail.python.org/mailman/listinfo/scikit-image > >
_______________________________________________ scikit-image mailing list scikit-image@python.org https://mail.python.org/mailman/listinfo/scikit-image
