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

[Terry]

On Mon, Jun 8, 2020 at 11:14 AM Ralf Gommers <ralf.gommers at gmail.com> wrote:

>
>
> 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.
>

Like this?
distances\return_distances |      False       |      True
---------------------------+------------------+-----------------
          None             |     nothing      | return distances
        ndarray            |     nothing      | populate ndarray

indicies\return_indicies   |      False       |      True
---------------------------+------------------+-----------------
          None             |     nothing      | return distances
        ndarray            |    *nothing*     | populate ndarray


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).
>

This seems like it would warrant deprecation, since the only way to do an
in-place `distance` calculation is changing.
distances\return_distances |      False       |      True
---------------------------+------------------+-----------------
          None             |     nothing      | return distances
        ndarray            |*populate ndarray*|     *error*

indicies\return_indicies   |      False       |      True
---------------------------+------------------+-----------------
          None             |     nothing      | return distances
        ndarray            | populate ndarray |     *error*

>
> Cheers,
> Ralf
>
>
If we're still considering an API change, I'd rather change return_* to
calculate_* to get the names match the behavior instead of visa-versa.

distances\*calculate_distances* | False       |      True
-----------------------------+----------------+-----------------
          None             |     nothing      | return distances
        ndarray            |     nothing      | populate ndarray

indicies\*calculate_indicies* |   False       |      True
----------------------------+-----------------+-----------------
          None             |     nothing      | return distances
        ndarray            |    *nothing*     | populate ndarray

Cheers,
Terry



>>
>> 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
>>
> _______________________________________________
> 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/20200609/b7603d3c/attachment-0001.html>