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

feature/issue 1731 #1753

Closed

Conversation

ChangsenJiang
Copy link

@ChangsenJiang ChangsenJiang commented Mar 13, 2024

DEP: remove the binomial_exact() function and the related tests , fixes #1731
Remove the cogent3.maths.stats.binomial_exact() function, and removed the related test_binomial_series(), test_binomial_exact(), test_binomial_exact_floats(), and test_binomial_exact_errors()

cogent3#1731

In this commit, the binomial_exact() function was removed and the related test_binomial_series(),  test_binomial_exact(), test_binomial_exact_floats(), and test_binomial_exact_errors() were also been removed
Copy link
Collaborator

@GavinHuttley GavinHuttley left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

When marking a function to be discontinued we limit our changes to applying the decorator and giving the reason (including referring users to a replacement in another project). In other words, we do not actually change our code within the function since we are going to be deleting it.

src/cogent3/maths/stats/distribution.py Outdated Show resolved Hide resolved
src/cogent3/maths/stats/distribution.py Outdated Show resolved Hide resolved
@ChangsenJiang ChangsenJiang marked this pull request as draft March 13, 2024 01:32
cogent3#1731

In this new commit, we revert the change within the function to the original, and delete the new import
@ChangsenJiang ChangsenJiang marked this pull request as ready for review March 13, 2024 01:46
In this issue, the version information in the decorator was changed from the wrong "2024.6" to "2024.9". The format was also changed to the correct form.
In this commit,  linting was performed to correct the argument format.
In this commit, the "pragma: no cover" comment was added ,and the docstring within the function was changed to """being removed"""
@ChangsenJiang ChangsenJiang marked this pull request as draft March 13, 2024 04:30
@ChangsenJiang ChangsenJiang marked this pull request as ready for review March 13, 2024 04:30
@ChangsenJiang
Copy link
Author

Now I revert the change within the function to the original and delete the new imports. Please have a look : )

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 8259402581

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 91.617%

Totals Coverage Status
Change from base Build 8151954237: -0.001%
Covered Lines: 29618
Relevant Lines: 32328

💛 - Coveralls

@ChangsenJiang
Copy link
Author

ChangsenJiang commented Mar 25, 2024

I have already made those required changes 2 weeks ago. Please have a look : )

Copy link
Collaborator

@GavinHuttley GavinHuttley left a comment

Choose a reason for hiding this comment

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

Thanks for your effort, but I can't accept this PR as is.

First, the replacement function in scipy you have identified is incorrect. It should be scipy.stats.binomtest. This means you would need to update your deprecation reason and, in doing so, correct the spacing between words.

Second, you would need to identify all uses of binomial_exact() in cogent3 and replace them with correct usage of the scipy function.

Finally, be sure and delete imports of binomial_exact() since it is no longer being used.

@GavinHuttley
Copy link
Collaborator

Closing due to lack of response to review. But thanks for your effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mark binomial_exact() function for removal
3 participants