Mailman 3 April 2017 - scikit-image

Turning mailing list reply-to-list on by default
by Juan Nunez-Iglesias 14 Apr '17

14 Apr '17

Hi all, There is general debate about whether mailing list messages should reply to the list or to the author by default. My view is that replies to list messages should go to the list, and replies only to the author are the exception. This would also be consistent behaviour with the lists of Numba, NumPy, SciPy, and scikit-learn. I intend to update the list settings in a couple of days unless I get some very strenuous opposition to this message. And even then I might do it anyway. =P Just a heads-up! Juan.

2 1

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Nathaniel Smith 14 Apr '17

14 Apr '17

On Thu, Apr 13, 2017 at 3:28 PM, Nelle Varoquaux <nelle.varoquaux(a)gmail.com> wrote: > Hello, > > For style-changes (such as pep8/flake8) elements, I find that very often new > contributors don't know they can configure their text editors to display the > pep8/pyflakes violations. Very often, it is a question of mentoring them > through the process of configuring their text editors. > Recently, I've been either creating pull requests on people's branch or > pushing to the contributor's branch just before merging. I wonder if it would be worthwhile to have a bot that automatically pushes formatting fixes to submitted PRs (using autopep8 or yapf). -n -- Nathaniel J. Smith -- https://vorpus.org

1 0

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Juan Nunez-Iglesias 14 Apr '17

14 Apr '17

Hey Vighnesh, I'm -1 on this level of optimism. =P I think code in master should reflect with probability 1 the best that we can make it. What that means is that "quick fix" PRs can get merged because the maintainer who merges it can issue a fix instantly. For large fixes, the most likely scenario is that it stays in the code for a long time, and then come release time there is a huge burden for the maintainers. Regarding new contributors pitching in, my take is that most new contributors are using the stable release, and so are unlikely to have even encountered the "dirty" function. Finally, for "single use" functions, it is straightforward enough for one to grab them off the contributor's fork, I think. Juan. On 14 Apr 2017, 2:30 PM +1000, Vighnesh Birodkar <vighneshbirodkar(a)gmail.com>, wrote: > 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.soma(a)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. And the current PRs on manual segmentation and nD rescaling 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 <stefanv(a)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(a)python.org > > > > https://mail.python.org/mailman/listinfo/scikit-image > > > > > > _______________________________________________ > > > scikit-image mailing list > > > scikit-image(a)python.org > > > https://mail.python.org/mailman/listinfo/scikit-image > > >

2 1

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Juan Nunez-Iglesias 14 Apr '17

14 Apr '17

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. And the current PRs on manual segmentation and nD rescaling 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 <stefanv(a)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(a)python.org > https://mail.python.org/mailman/listinfo/scikit-image

4 3

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Nelle Varoquaux 13 Apr '17

13 Apr '17

> > For style-changes (such as pep8/flake8) elements, I find that very often > new contributors don't know they can configure their text editors to > display the pep8/pyflakes violations. Very often, it is a question of > mentoring them through the process of configuring their text editors. > > > > This seems like the kind of thing we can document in our contributor guide? > > http://scikit-image.org/docs/stable/contribute.html#stylistic-guidelines > > > That seems a fairly sensible place to document this :) > But considering this affect many projects in the scipy community, I am > wondering if we should not try to combine efforts and link to a common page > somewhere on the web (maybe hosted on the scipy-lecture notes website?) > I am happy to work on this, either for scikit-image specifically or to > create the webpage somewhere else then edit the scikit-image contribution > guidelines to link to this document. > > > Good idea; and thanks! > I just got the approval from Gael to add a chapter "Best development practices" to scipy lecture notes. That should cover setting up text editors and other elements. If you have any other ideas on what should be in that chapter, just email me. Cheers, N > > Stéfan > >

1 0

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Stefan van der Walt 13 Apr '17

13 Apr '17

On Thu, Apr 13, 2017, at 15:45, Nelle Varoquaux wrote: > On 13 April 2017 at 15:36, Stefan van der Walt > <stefanv(a)berkeley.edu> wrote: >> __ >> >> On Thu, Apr 13, 2017, at 15:28, Nelle Varoquaux wrote: >>> For style-changes (such as pep8/flake8) elements, I find that very >>> often new contributors don't know they can configure their text >>> editors to display the pep8/pyflakes violations. Very often, it is a >>> question of mentoring them through the process of configuring their >>> text editors. >> >> >> This seems like the kind of thing we can document in our >> contributor guide? >> >> http://scikit-image.org/docs/stable/contribute.html#stylistic-guidelines > > That seems a fairly sensible place to document this :) > But considering this affect many projects in the scipy community, I am > wondering if we should not try to combine efforts and link to a common > page somewhere on the web (maybe hosted on the scipy-lecture notes > website?) > I am happy to work on this, either for scikit-image specifically or to > create the webpage somewhere else then edit the scikit-image > contribution guidelines to link to this document. Good idea; and thanks! Stéfan

1 0

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Nelle Varoquaux 13 Apr '17

13 Apr '17

On 13 April 2017 at 15:36, Stefan van der Walt <stefanv(a)berkeley.edu> wrote: > On Thu, Apr 13, 2017, at 15:28, Nelle Varoquaux wrote: > > For style-changes (such as pep8/flake8) elements, I find that very often > new contributors don't know they can configure their text editors to > display the pep8/pyflakes violations. Very often, it is a question of > mentoring them through the process of configuring their text editors. > > > This seems like the kind of thing we can document in our contributor guide? > > http://scikit-image.org/docs/stable/contribute.html#stylistic-guidelines > That seems a fairly sensible place to document this :) But considering this affect many projects in the scipy community, I am wondering if we should not try to combine efforts and link to a common page somewhere on the web (maybe hosted on the scipy-lecture notes website?) I am happy to work on this, either for scikit-image specifically or to create the webpage somewhere else then edit the scikit-image contribution guidelines to link to this document. Cheers, N > > > Stéfan > > > _______________________________________________ > scikit-image mailing list > scikit-image(a)python.org > https://mail.python.org/mailman/listinfo/scikit-image > >

1 0

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Stefan van der Walt 13 Apr '17

13 Apr '17

On Thu, Apr 13, 2017, at 15:28, Nelle Varoquaux wrote: > For style-changes (such as pep8/flake8) elements, I find that very > often new contributors don't know they can configure their text > editors to display the pep8/pyflakes violations. Very often, it is a > question of mentoring them through the process of configuring their > text editors. This seems like the kind of thing we can document in our contributor guide? http://scikit-image.org/docs/stable/contribute.html#stylistic-guidelines Stéfan

1 0

Re: [scikit-image] How to move pull requests forward (devs, please read)
by Nelle Varoquaux 13 Apr '17

13 Apr '17

Hello, For style-changes (such as pep8/flake8) elements, I find that very often new contributors don't know they can configure their text editors to display the pep8/pyflakes violations. Very often, it is a question of mentoring them through the process of configuring their text editors. Recently, I've been either creating pull requests on people's branch or pushing to the contributor's branch just before merging. Cheers, N On 13 April 2017 at 00:41, Sourav Singh <ssouravsingh12(a)gmail.com> wrote: > Hello, > > I agree with most of the points put by Juan. Here are some points that I > would like to put forward- > > 1) If there is a PR which has a merge conflict in a single file, we can > tell the contributor to fix it through Github PR through the merge conflict > button. > > 2) If the problem of merge is severe, core devs can come forward to fix > the problem if the contributor is not a git expert. > > 3) For long standing PRs, first-time contributors could come forward and > handle the problem if they know what needs to be done. > > Comments are very welcome! > > Regards, > > Sourav > > Sent from my Phone.Excuse any mistakes and brevity. > > _______________________________________________ > scikit-image mailing list > scikit-image(a)python.org > https://mail.python.org/mailman/listinfo/scikit-image > >

1 0

Re: [scikit-image] Important change in testing environment
by Stefan van der Walt 12 Apr '17

12 Apr '17

On Wed, Apr 12, 2017, at 01:46, Egor Panfilov wrote: > We'd like to notify your that we have just finished a transition of > our testing environment from `nose` to `pytest` (see > https://github.com/scikit-image/scikit-image/pull/2468 for the > details). > > We kindly ask you to revisit your open Pull Requests on GitHub, > perform a rebase onto `master` branch of `scikit-image/scikit-image` > repo (see > http://scikit-image.org/docs/dev/contribute.html#divergence-between-upstrea…), > and rewrite your tests using `pytest` wherever is needed. > > Usage of the functions from `numpy.testing` in addition to `pytest` > ones is allowed, but, please, notice that some of them (such as > `assert_raises`) rely on `nose` under the hood, and, therefore, should > not be used. Thanks for the reminder, Egor. The changes will, hopefully, be minimal from the developer perspective. And we're working on another PR that exposes all of the test utilities under `skimage.util`, to simplify matters. Best regards Stéfan

1 0