[SciPy-Dev] ndimage grey morphology tickets

[Tony Yu]

On Tue, May 8, 2012 at 10:52 AM, Jonathan Helmus <jjhelmus at gmail.com> wrote:

> Tony Yu wrote:
> >
> >
> > On Mon, May 7, 2012 at 3:58 PM, Tony Yu <tsyu80 at gmail.com
> > <mailto:tsyu80 at gmail.com>> wrote:
> >
> >
> >
> >     On Mon, May 7, 2012 at 3:38 PM, Jonathan Helmus
> >     <jjhelmus at gmail.com <mailto:jjhelmus at gmail.com>> wrote:
> >
> >         All,
> >
> >         Trac tickets #1135, #1281 and #1498 point out bugs in the
> >         ndimage.grey_dilation and grey_erosion function.  I've started
> >         a branch
> >         that tries to address these issues:
> >         https://github.com/jjhelmus/scipy/tree/grey_morphology-fix
> >
> >         This branch currently passes the tests in test_ndimage.py and the
> >         dilation_test.py file attached to ticket 1135, but I am not
> >         certain on
> >         two issues that I was hoping someone here might be able to
> >         comment on them:
> >
> >         1.  Should there be a sanity check on the shape of footprint,
> >         structure
> >         and size when more than one is provided?
> >         2.  Ticket #1281 points out that grey_erosion does not have the
> >         parameter checking that grey_dilation has.  I added these
> >         checks but
> >         noticed that  to pass tests structure and footprint should not be
> >         reversed and the origin negated.  Is this correct?  If so the
> >         commented
> >         out lines in the branch should will be deleted.
> >
> >         If this would be better discussed in a pull request I'd be
> >         happy to make
> >         one.
> >
> >         http://projects.scipy.org/scipy/ticket/1135
> >         http://projects.scipy.org/scipy/ticket/1281
> >         http://projects.scipy.org/scipy/ticket/1498
> >
> >
> >         Cheers,
> >
> >            -Jonathan Helmus
> >
> >     Hi Jonathan,
> >
> >     I recently submitted a fix for ticket 1135
> >     <https://github.com/scipy/scipy/pull/199/files>, but I realize now
> >     that I should have pinged the list. I didn't actually know about
> >     tickets 1281 or 1498. I think the PR I submitted should also take
> >     care of 1498, but I don't think 1281 is actually a bug.
> >
> >     IIRC, the reason for the difference is that dilate shifts the
> >     origin of the structuring element/footprint if its size is
> >     even-numbered (i.e. doesn't have a "center" pixel). This shift
> >     makes dilation and erosion reversible---otherwise applying one,
> >     then the other would shift features. Are you sure the changes to
> >     `grey_erosion` are necessary?
> >
> >     Also, I trimmed down the tests from PR 1135 (which seem to be the
> >     same tests in your branch) so that they only test for the
> >     submitted change. I think I originally wrote the tests (it's been
> >     more than 2 years so I could be wrong about that), and I was just
> >     testing everything related to the change. I trimmed it down
> >     because some tests replicated existing tests (and
> >     `test_ndimage.py` is already really long).
> >
> >     I haven't had a chance to carefully look over your branch to
> >     compare, but I will tonight.
> >
> >     -Tony
> >
> >
> > After a closer look, I'm pretty sure your changes to `grey_dilate` do
> > essentially the same thing as PR #199
> > <https://github.com/scipy/scipy/pull/199/files>.
> >
> > As I mentioned in the previous email, the reason `grey_dilate` isn't
> > symmetric in appearance to `grey_erosion` is because it needs to shift
> > the origin of `structure`/`footprint`.
> >
> > Also, after closer review, I'm pretty sure the changes to
> > `grey_erosion` are unnecessary. Could you take a closer look, just to
> > make sure? In particular, I believe the changes to `grey_erosion` are
> > handled by `filter._min_or_max_filter`
> > <
> https://github.com/scipy/scipy/blob/master/scipy/ndimage/filters.py#L811>.
> > (The preprocessing of arguments---e.g. calls to `asarray` and
> > `_normalize_sequence`--- in `grey_dilation` is required to shift the
> > origin, but this processing is redone in `_min_or_max_filter`.)
> >
> > -Tony
> >
> Tony,
>
>    I looked at your pull request and you are correct, no change is
> needed in grey_erosion and our two modification to grey_dilation have
> the same effect.  As I mentioned in the pull request comment the
> following might be good to add:
>
> Raise a RuntimeError if footprint, structure and size are all None.
> Current ndimage.grey_dilation([1]) raises a TypeError at line 1428,
> which is not as helpful.  If this sanity check is added something
> similar might be added to grey_erosion, which currently raises a
> RuntimeError at line 816 complaing about no footprint, but probably
> should mention that structure or size can also be provided.
>
> Update the doc for grey_dilation and grey_erosion to mention that size
> is optional if footprint or structure is provided.
>
> I'll watch your pull request and stop working on my branch as your
> branch is more mature.
>
> Cheers,
>
>    -Jonathan Helmus
>


Thanks for looking over the PR, Jonathan. I added one commit to check for
the no-input case, and a second commit to update the docstring for (grey_)
dilate, erosion, opening, closing, and morphological_gradient (I also
verified that these functions do not require `size` to be specified if
`structure` or `footprint` is specified).

Could a dev with commit rights take a look at this? The main change is 4
lines of code (see first
commit<https://github.com/tonysyu/scipy/commit/479ae86e4319251dcb5d39783d8a90de9422f5b9>).
The rest of the changes are just docstrings and tests.

Thanks,
-Tony
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scipy-dev/attachments/20120508/d5ebab49/attachment.html>