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

Implement checks for unannotated categorical column values + unused annotated missing values #115

Merged
merged 8 commits into from
Apr 13, 2023

Conversation

alyssadai
Copy link
Contributor

@alyssadai alyssadai commented Apr 12, 2023

Closes #107, closes #109

Two new examples also added for testing, example 9 (.tsv has unannotated categorical column values) and 10 (.json has unused "MissingValues"). Minor noticed typos also fixed.

@github-actions
Copy link

github-actions bot commented Apr 12, 2023

Pull Request Test Coverage Report for Build 4686162992

  • 39 of 39 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 97.912%

Totals Coverage Status
Change from base Build 4608993501: 0.2%
Covered Lines: 469
Relevant Lines: 479

💛 - Coveralls

@surchs surchs self-requested a review April 12, 2023 20:25
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Looks good to me @alyssadai! I think we should keep the tests a bit more loose with respect to the exact warning message we throw (left comments) so we keep them focused more on the functionality than the exact implementation.

I also left some 🍒 comments for you to consider.

bagel/tests/data/README.md Outdated Show resolved Hide resolved
bagel/tests/data/example10.json Outdated Show resolved Hide resolved
bagel/tests/data/example9.tsv Outdated Show resolved Hide resolved
bagel/tests/test_cli_pheno.py Outdated Show resolved Hide resolved
bagel/tests/test_cli_pheno.py Show resolved Hide resolved
bagel/pheno_utils.py Show resolved Hide resolved
bagel/pheno_utils.py Outdated Show resolved Hide resolved
bagel/pheno_utils.py Outdated Show resolved Hide resolved
bagel/pheno_utils.py Show resolved Hide resolved
bagel/pheno_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Looks great, good to go 🧑‍🍳

@alyssadai alyssadai merged commit d5762fc into main Apr 13, 2023
@alyssadai alyssadai deleted the feat-109/discrete-values-check branch April 13, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants