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] Improve distribution tests #57

Merged
merged 7 commits into from
Sep 9, 2023

Conversation

Alex-JG3
Copy link
Contributor

@Alex-JG3 Alex-JG3 commented Sep 8, 2023

Reference Issues/PRs

Closes #55, see also #21.

What does this implement/fix? Explain your changes.

  1. Adds a test to check whether the log of the pdf function is similar to the log_pdf function for each distribution.
  2. Adds a test to check that the ppf is the inverse of the cdf function. This test required the change described in 1 to use np.allclose.
  3. Adds a test to check that pandasoutputs of methods have all numeric dtype-s

Also fixes one instance that would have failed 2, 3:

  • Changes the method for converting datatypes in the _apply_per_ix function of the Empirical distribution. Switches to use to_numeric instead of convert_dtypes.

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
    the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)
  • New public functionality has been added to the API Reference

* Add test to check log of the pdf is the same as the log_pdf function.
* Add test to check the ppf is the inverse of the cdf.
@Alex-JG3 Alex-JG3 changed the title [ENH]Improve distribution tests [ENH] Improve distribution tests Sep 8, 2023
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Non-blocking change suggestions:

  • I think you should add yourself to the authors field of test_all_distrs for this
  • this does not seem to directly check the output dtypes that the change to _apply_per_ix fixes. Should a direct check be added to _check_output_format?

@fkiraly
Copy link
Collaborator

fkiraly commented Sep 8, 2023

sorry, the failures are due to the recent upgrade of the precommit hooks, it now likes and dislikes slightly different things. Should be fixed with an update from main.

@fkiraly
Copy link
Collaborator

fkiraly commented Sep 8, 2023

there´s one left, just fixing: #62

merged and pushed to this branch - sorry for the inconvenience. Should be fixed now.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Patch coverage: 84.00% and project coverage change: +1.81% 🎉

Comparison is base (4f46ee9) 68.81% compared to head (cd78cf8) 70.63%.
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   68.81%   70.63%   +1.81%     
==========================================
  Files          93       93              
  Lines        4881     4904      +23     
  Branches      886      890       +4     
==========================================
+ Hits         3359     3464     +105     
+ Misses       1294     1204      -90     
- Partials      228      236       +8     
Files Changed Coverage Δ
skpro/distributions/tests/test_all_distrs.py 92.95% <82.60%> (-2.05%) ⬇️
skpro/distributions/empirical.py 88.23% <100.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Alex-JG3
Copy link
Contributor Author

Alex-JG3 commented Sep 8, 2023

Should be fixed now.

Thanks for sorting this out!

@fkiraly
Copy link
Collaborator

fkiraly commented Sep 9, 2023

Thanks for sorting this out!

Well, sorry for messing it up in the first place...

@fkiraly fkiraly merged commit 0d8bd68 into sktime:main Sep 9, 2023
27 checks passed
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.

[BUG] The cdf for the Empirical distribution returns a dataframe of objects
3 participants