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

Add check for complete categories when constructing contingency tables #184

Merged
merged 14 commits into from Mar 17, 2022

Conversation

jarq6c
Copy link
Collaborator

@jarq6c jarq6c commented Mar 3, 2022

This adds a check and corrects missing boolean categories in Categorical series input to compute_contingency_table.

Additions

  • Checks for both True and False in observed and simulated input series.

Removals

  • None

Changes

  • Update original test to use categorical series instead of raw categories per the original docstring and intended use of compute_contingency_table.

Testing

  1. Four new test scenarios for compute_contingency_table that check for all True and all False cases.

Notes

Todos

  • None

Checklist

  • PR has an informative and human-readable title
  • PR is well outlined and documented. See #12 for an example
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (see CONTRIBUTING.md)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output) using numpy docstring formatting
  • Placeholder code is flagged / future todos are captured in comments
  • Reviewers requested with the Reviewers tool ➡️

@jarq6c jarq6c added the bug Something isn't working label Mar 3, 2022
@jarq6c jarq6c self-assigned this Mar 3, 2022
@jarq6c jarq6c requested a review from hellkite500 March 3, 2022 21:33
@jarq6c
Copy link
Collaborator Author

jarq6c commented Mar 4, 2022

Tagging @aaraney

@jarq6c
Copy link
Collaborator Author

jarq6c commented Mar 9, 2022

@hellkite500 @aaraney any feedback on this PR?

@jarq6c
Copy link
Collaborator Author

jarq6c commented Mar 15, 2022

@hellkite500 @aaraney Following last week's meeting, I replaced all the complicated categorical checks with a simple pandas.Series creation. This removed the need for a lot of code and as a nice side-effect made compute_contingency_table compatible with any array-like, instead of just pandas.Series. This means the compute_contingency_table interface is more like the other metrics.

I was also able to remove a step from the README example since the categorical conversion is now covered by the _validation.convert_to_boolean_categorical_series method.

The only warning that's still raised is if the method encounters something that isn't one of the prescribed categories. These values are treated as NaN and the user is warned.

@jarq6c jarq6c requested a review from hellkite500 March 15, 2022 19:23
@hellkite500
Copy link
Member

Nice work on the refactor, by the way! Much simpler, and even more general! I like it!

@jarq6c
Copy link
Collaborator Author

jarq6c commented Mar 15, 2022

Nice work on the refactor, by the way! Much simpler, and even more general! I like it!

Thanks for the review!

@hellkite500
Copy link
Member

Approved, but I'll give @aaraney a chance to look it over before merging.

@jarq6c jarq6c merged commit 5f2dccf into NOAA-OWP:main Mar 17, 2022
@jarq6c jarq6c deleted the ctab_issue183 branch March 17, 2022 18:55
@jarq6c
Copy link
Collaborator Author

jarq6c commented Mar 17, 2022

This has been published to PyPI.

@aaraney
Copy link
Member

aaraney commented Mar 20, 2022

sorry for being slow to get to this. having looked at it now, I would have approved the merge. like @hellkite500's noted, I think the refactor was done nicely. Nice work!

@jarq6c
Copy link
Collaborator Author

jarq6c commented Mar 21, 2022

sorry for being slow to get to this. having looked at it now, I would have approved the merge. like @hellkite500's noted, I think the refactor was done nicely. Nice work!

Thanks @aaraney !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants