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

Cramers v #1446

Merged
merged 18 commits into from May 16, 2022
Merged

Cramers v #1446

merged 18 commits into from May 16, 2022

Conversation

JKL98ISR
Copy link
Member

closes #1272

@JKL98ISR JKL98ISR added the feature Feature update or code change to the package label May 15, 2022
@JKL98ISR JKL98ISR self-assigned this May 15, 2022
@JKL98ISR JKL98ISR requested review from a team, ItayGabbay and shir22 as code owners May 15, 2022 18:50
@@ -107,6 +117,7 @@ def __init__(
else:
raise DeepchecksValueError('sort_feature_by must be either "feature importance" or "drift score"')
self.n_top_columns = n_top_columns
self.categorical_drift_method = categorical_drift_method
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe validate the value in the init?

Copy link
Member Author

Choose a reason for hiding this comment

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

in similar cases we also don't check on init so I don't think this case should be an exception

deepchecks/utils/distribution/drift.py Outdated Show resolved Hide resolved
deepchecks/utils/distribution/drift.py Outdated Show resolved Hide resolved
@ItayGabbay
Copy link
Member

Approved the docs part.

@JKL98ISR JKL98ISR enabled auto-merge (squash) May 16, 2022 13:20
@JKL98ISR JKL98ISR merged commit d80f851 into main May 16, 2022
@delete-merged-branch delete-merged-branch bot deleted the cramers_v branch May 16, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature update or code change to the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] Add Cramer's V in addition to PSI for all categorical drift
4 participants