[SciPy-Dev] Proposal: Deprecate confusing flag params in ndimage.distance_transform_*
Terry
terry.y.davis+scipy at gmail.com
Fri Jun 19 14:36:39 EDT 2020
Hi Ralf,
Did you see my last message? I just realized that the entire message body
was under the gmail's fold, so it may have appeared blank to you. (...how
do I get rid of the fold?)
-Terry
On Tue, Jun 9, 2020 at 9:23 AM Terry <terry.y.davis+scipy at gmail.com> wrote:
>
> 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/20200619/15a3b0a1/attachment-0001.html>
More information about the SciPy-Dev
mailing list