Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: stats.fisher_exact: convert output tuple to Bunch #16407

Merged
merged 4 commits into from Jun 16, 2022

Conversation

JozsefKutas
Copy link
Contributor

Reference issue

#16364
#13118

What does this implement/fix?

#16364 suggests that all statistical functions that currently return tuples should return Bunch objects. This PR converts the tuple returned by stats.fisher_exact to a Bunch.

@mdhaber
Copy link
Contributor

mdhaber commented Jun 14, 2022

Thanks @JozsefKutas. At first glance, this looks great! Let's see what another maintainer thinks of gh-16387, and if that looks OK to them, I'll review/merge this and other follow-ups.

@tupui tupui added maintenance Items related to regular maintenance tasks scipy.stats labels Jun 15, 2022
Comment on lines 4480 to 4481
FisherExactResult = _make_tuple_bunch('FisherExactResult',
['statistic', 'pvalue'], [])
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have a common "tuple bunch" as I said in the other PR from @mdhaber. Otherwise I am +1 on doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, what should the name be? StatsTestResult?

Copy link
Member

Choose a reason for hiding this comment

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

Might be too general. What about HypothesisTestResult? Not sure if there would be something that would just say p-value and statistic. SignificanceTestResult maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is combine_pvalues well-described by "hypothesis test"? Not sure, but I like a slight tweak of what you wrote: I think SignificanceResult is a good name for an object with just attributes statistic and pvalue.

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with that then 👍

@mdhaber
Copy link
Contributor

mdhaber commented Jun 15, 2022

OK I just changed gh-16387 to SignificanceResult; please merge that if it looks good. Then we can update this and continue. Let's leave SignificanceResult out of the public API for now, but at some point we may wish to expose it.

Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

Merged main so we could use SignificanceResult and parametrized test.

@JozsefKutas do these changes look good to you? If so, I'll merge, and we can continue down the list - just like this is great, I think.

@JozsefKutas
Copy link
Contributor Author

Yup, that seems fine to me.

For the other statistical functions, both chi2_contingency and median_test return other results as well as the statistic/p-value. In these cases would you prefer a function-specific result object, as per the original PR?

@mdhaber
Copy link
Contributor

mdhaber commented Jun 15, 2022

would you prefer a function-specific result object, as per the original PR?

Yup, I think so. We can consider consolidating similar result objects later.

@mdhaber
Copy link
Contributor

mdhaber commented Jun 16, 2022

Thanks @JozsefKutas @tupui!

@mdhaber mdhaber merged commit c820ec7 into scipy:main Jun 16, 2022
@mdhaber mdhaber added this to the 1.10.0 milestone Jul 7, 2022
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.

None yet

3 participants