Skip to content

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented Dec 13, 2023

Reference issue

What does this implement/fix?

stats.moment has a parameter named moment. Given that the description is

Order of central moment that is returned

and the name of the equivalent argument of scipy.stats.rv_generic.moment is order,

image

I think the name order would be more appropriate.

This PR renames the parameter in a way that maintains backward compatibility (i.e. use of keyword-argument 'moment' is still allowed as an alias).

Additional information

  • This would need approval from the mailing list.
  • We could consider deprecating use of kwarg moment instead of leaving it as an alias, but I am not necessarily pushing for it.
  • I could refactor the code to actually use order instead of moment, but I chose instead to let moment = order at the top to minimize the diff.

@mdhaber mdhaber added scipy.stats maintenance Items related to regular maintenance tasks labels Dec 13, 2023
@mdhaber mdhaber requested a review from j-bowhay December 13, 2023 08:15
@j-bowhay
Copy link
Member

Seems reasonable although I would prefer to deprecate rather than have two ways of doing the same thing. Is there a specific reason for not? Will wait for the mailing list post before reviewing in detail

@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 17, 2023

Is there a specific reason for not?

Well, mostly because the keyword moment is not broken. The motivation for renaming rv_generic from n to order was that there was a name collision between the method parameter n and the shape parameter n of some distributions, so something needed to change. Here, we'd only be improving code clarity and consistency, which might not be enough to justify the backward compatibility break.

I'm not sure I can call for a deprecation right now, but I wouldn't oppose it.

@rgommers
Copy link
Member

+1 for merging as is (is moment= alias tested? couldn't tell from the diff), and not deprecating. This is more or less a cosmetic change, so deprecating doesn't seem quite right.

@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 28, 2023

@rgommers Oops, I got carried away and changed the name everwhere. I added an explicit test.

@steppi
Copy link
Contributor

steppi commented Dec 30, 2023

+1 for merging as is without deprecating.

Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the example to use order instead of moment

@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 30, 2023

Thanks @j-bowhay. Did a search and replace this time; I think I got the last of the ones that are supposed to be changed.

@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 30, 2023

# returns it.
# Currently this leads to a slight inconsistency: when the input array is
# empty, there is no distinction between the `moment` function being called
# with parameter `moments=1` and `moments=[1]`; the latter *should* produce
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry missed this last time around. Does this comment also need updating? It looks like there might have originally been a moments->moment typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mdhaber, will leave a few days before merging in case there is anything from the mailing list

@mdhaber mdhaber requested a review from j-bowhay January 19, 2024 16:51
@j-bowhay j-bowhay merged commit 647d1cb into scipy:main Jan 19, 2024
@j-bowhay
Copy link
Member

Thanks for the reminder @mdhaber, that was a long few days!

@mdhaber mdhaber added this to the 1.13.0 milestone Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants