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@gmail.com> wrote:

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

...

On Sun, Jun 7, 2020 at 6:47 AM Terry <terry.y.davis+scipy@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@gmail.com> wrote:

...

On Wed, Jun 3, 2020 at 8:34 AM Terry <terry.y.davis+scipy@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@python.org https://mail.python.org/mailman/listinfo/scipy-dev

_______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev

_______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev

_______________________________________________ SciPy-Dev mailing list SciPy-Dev@python.org https://mail.python.org/mailman/listinfo/scipy-dev