[SciPy-Dev] Proposal: Deprecate confusing flag params in ndimage.distance_transform_*

[Ralf Gommers]

On Sun, Jun 7, 2020 at 6:47 AM Terry <terry.y.davis+scipy at gmail.com> wrote:

> Hi Ralf,
>
> Thanks for the feedback.
>
> To be clear, my proposal is to drop the return_* args entirely.
>

I misread that, I thought you wanted to drop the other two args instead.
Either way, I don't think that changes my feedback. Overloading two
keywords more isn't a real improvement.

If that’s not desirable, we could also decouple return_* from *, so the arg
> names would be more descriptive, but perhaps this would not be useful.
>
> If you’d rather proceed with the change you described, my only questions
> are whether the fix warrants a deprecation cycle, or does it seem low risk
> enough to just include in the release notes, and is a test necessary?
>

It doesn't need a deprecation, that's just a bug fix so the change can be
made straight away. To guard against misuse, you could add a check that if
a return_* is True but the corresponding * is an ndarray, raise an error
(because the two keywords are in conflict).

Cheers,
Ralf


>
> Either way, I’d be happy to add it to the PR I’ve got open.
>
> Cheers,
> Terry
>
> On Sat, Jun 6, 2020 at 15:19 Ralf Gommers <ralf.gommers at gmail.com> wrote:
>
>>
>>
>> On Wed, Jun 3, 2020 at 8:34 AM Terry <terry.y.davis+scipy at gmail.com>
>> wrote:
>>
>>> Hi all,
>>>
>>> I would like to remove the parameters `return_distances` and
>>> `return_indicies`, because their names are misleading and behaviors are
>>> different.
>>>
>>> While working on this PR: https://github.com/scipy/scipy/pull/12144
>>> I realized that the distance_transform_* functions had confusing and
>>> complex interactions between the `return_distances`/`distances` and
>>> `return_indicies`/`indicies` pairs (the feature transform is always
>>> calculated because it's needed to calculate the distance transform).
>>>
>>
>> I agree that that's pretty weird. The `return_something` keyword pattern
>> is present in a number of places in SciPy. It's not great, but we live with
>> it. The combo keywords in these functions where you can provide an in-place
>> argument as well is not present anywhere else as far as I know.
>>
>>
>>> Here's how the the behavior of each pair differs:
>>> distances\return_distances |      False       |      True
>>> ----------------------------------------------+-----------------
>>>           None             |     nothing      | return distances
>>>         ndarray            |     nothing      | populate ndarray
>>>
>>> indicies\return_indicies   |      False       |      True
>>> ----------------------------------------------+-----------------
>>>           None             |     nothing      | return distances
>>>         ndarray            | populate ndarray | populate ndarray
>>>
>>
>> The indices (False, ndarray) case seems wrong, that should also do
>> nothing. For the rest, this is weird but at least consistent.
>>
>>
>>> Here's the proposed API:
>>> distances:
>>> ndarray - put distance transform here, don't return it
>>> Truthy - Return the distance transform
>>> Falsey - Don't return the distance transform
>>>
>>> indicies:
>>> ndarray - put feature transform here, don't return it
>>> Truthy - Return the feature transform
>>> Falsey - Don't return the feature transform
>>>
>>
>> I don't think that's much better to be honest, that makes the `return_*`
>> keywords even more weird. I'd prefer just fixing the one inconsistency I
>> pointed out above and leaving it at that.
>>
>> Thanks for working on this Terry.
>>
>> Cheers,
>> Ralf
>>
>>
>>
>>> This should simplify the documentation and code, and make it easier to
>>> reason about these behaviors.
>>> If this sounds reasonable, I'd also be happy to make similar changes to
>>> any other parts of scipy that have multiple optional returns/outputs.
>>>
>>> Regards,
>>> Terry
>>> _______________________________________________
>>> SciPy-Dev mailing list
>>> SciPy-Dev at python.org
>>> https://mail.python.org/mailman/listinfo/scipy-dev
>>>
>> _______________________________________________
>> SciPy-Dev mailing list
>> SciPy-Dev at python.org
>> https://mail.python.org/mailman/listinfo/scipy-dev
>>
> _______________________________________________
> SciPy-Dev mailing list
> SciPy-Dev at python.org
> https://mail.python.org/mailman/listinfo/scipy-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scipy-dev/attachments/20200608/c19fd7d6/attachment.html>