[SciPy-Dev] ndimage grey morphology tickets
Tony Yu
tsyu80 at gmail.com
Tue May 8 15:02:25 EDT 2012
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>
More information about the SciPy-Dev
mailing list