Add new flag to convex_hull_image and grid_points_in_poly

[dudulasry]

Description

Closes #6510

This PR addresses the problem described in #6510. The cause for this problem was also discussed in #3892, which was later addressed in this PR.

The solution that this PR offers is to add a flag to convex_hull_image, grid_points_in_poly and _grid_points_in_poly, offering to return the raw labels as well. This will allow the users of these functions to transform the raw labels array into a boolean array themselves.

Checklist

• Docstrings for all functions
• Gallery example in ./doc/examples (new features only)
• Benchmark in ./benchmarks, if your changes aren't covered by an
  existing benchmark
• Unit tests
• Clean style in the spirit of PEP8
• Descriptive commit messages (see below)

For reviewers

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.
• There is a bot to help automate backporting a PR to an older branch. For
  example, to backport to v0.19.x after merging, add the following in a PR
  comment: @meeseeksdev backport to v0.19.x
• To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
  can be removed and then added again. The benchmark output can be checked in
  the "Actions" tab.

[pep8speaks]

Hello @dudulasry! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-09-12 22:10:13 UTC

[rfezzani]

Thank you @dudulasry for your contribution ;)
When @grlee77 suggested adding a flag to the the grid_point_in_poly, I read it as "Adding a flag to specify whether a vertices/edges are considered inside or outside the polygone"... Am I wrong @grlee77 ?
But providing a new function grid_point_in_poly_label that returns the points labels would be a great addition 😉

[rfezzani]

Thank you @dudulasry! We are pretty close now 😉
I just disagree with the behavior introduced by the return_labels argument in the convex_hull_image function.

[rfezzani]

That's great! Thank you again @dudulasry 😉
Just a last little thing: the docstring still mentions return_labels 😅

[dudulasry]

That's great! Thank you again @dudulasry 😉 Just a last little thing: the docstring still mentions return_labels 😅

Thanks for your fast code review. I updated the docstring one more time. Let me know if the PR is good now.
BTW, is there a documentation anywhere regarding releasing new versions for this project? I'd like to know when are you guys planning on releasing a new version?

[rfezzani]

I just left a minor docstring fix, but the PR is approved from my side. Thank you @dudulasry 🎉
We now need a second @scikit-image/core approval for the changes to be included in next release.

BTW, is there a documentation anywhere regarding releasing new versions for this project? I'd like to know when are you guys planning on releasing a new version?

There is no such public documentation, but I am maybe mistaken here. @scikit-image/core may help us on this question 😉

[stefanv]

This looks basically good to go. @scikit-image/core any opinion on whether we should add a new function (here called grid_point_in_poly_label, which is a bit confusing given the meaning of "labels" in the context of skimage), or whether we should add an argument such as return_edge_type to grid_point_in_poly instead?

[rfezzani]

@stefanv, for recall, I point you to one of your comments concerning the use of flags to determine output types.
I am not particularly in favour of introducing the return_edge_type flag.

[stefanv]

Like I wrote in the comment, I don't know whether there are very many good options in cases like these. So far, our API decisions have leaned towards including it in the same function, as seen by a grep for return_ in the code.

[dudulasry]

@stefanv @rfezzani
IMO, it's okay to keep the logic in one function (grid_points_in_poly) and add a flag to it (return_labels). It doesn't add much complexity to grid_points_in_poly, since it's a very short function. It'd be relatively easy to read a long and understand what is going on.

In that case, convex_hull_image can use its own abstract flag (include_borders) and internally use return_labels to select the relevant pixels.

[soupault]

I am personally in favor of merging the functions into one, since they implement pretty much the same logic. As an alternative to the suggested earlier, the argument could be binarize=True, which does thresholding internally by labels>=0. When binarize=False, the extended (0,1,2,3) labels array is returned.

[lagru]

To throw one more idea out there: how do you feel about adding a parameter similar to out= which is used by many NumPy functions? E.g. in this case something like *, label_array= or *, label_result=?

In my opinion it can be a very explicit way to say "I want this optional thing stored here while being less problematic than modifying what is returned and easier to maintain than a separate function with duplicated documentation, tests, ... However, there is the disadvantage of requiring and validating a container as input to the parameter or a true callback. I'm curious how that idea seems to you in general.

[rfezzani]

I took the habit to avoid using flags to control function's output type or count since reading previously cited @stefanv comment, as I consider it a reasonable convention. The deviation from the rule made in the past isn't a good justification to repeat this deviation.
Now, it seems that this convention is not conventional 😅...

I 👍 binarize (or as_mask like proposed by @kne42) VS return_labels or return_edge_type.
@lagru your suggestion is interesting, as it offers a memory-use optimization and a new option to tackle this kind problems 😉.
For the memory optimization to be effective, the _pnpoly._grid_points_in_poly cython function must accept an out parameter...

[mkcor]

I would prefer keeping only one function and adding a binarize parameter defaulting to True (i.e., current behaviour). Otherwise it looks like duplicate code...

Thanks @dudulasry, your change is very nice insofar as it adds flexibility, letting the end-user binarize if/as they wish.

[stefanv]

I took the habit to avoid using flags to control function's output type or count since reading previously cited @stefanv comment, as I consider it a reasonable convention. The deviation from the rule made in the past isn't a good justification to repeat this deviation. Now, it seems that this convention is not conventional sweat_smile...

@rfezzani That quote that you cite is not in defense of your argument. What I said there is: several people have told me they don't like the pattern, but the only alternative suggested feels even worse.

There is some justification in repeating the pattern: consistency.

The strongest argument I can see against the pattern is that it makes typing very difficult.
Perhaps @jni has looked into that more?

[stefanv]

The proposed binarize option works for me.

[dudulasry]

@stefanv @rfezzani @mkcor Hi guys, I removed the extra function and added a binarize flag. Let me know what you think.

[rfezzani]

Just found few bad imports 😉

[dudulasry]

@rfezzani Fixed :)

[rfezzani]

LGTM, thank you @dudulasry 😉

[lagru]

Thanks @dudulasry, apart from two tweaks to the docstrings I'm happy to approve this.

[dudulasry]

Thanks @dudulasry, apart from two tweaks to the docstrings I'm happy to approve this.

@lagru Hi, I applied your suggestions. Thanks for the feedback :)

[lagru]

Merging. Thanks @dudulasry! 🎉