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

Return details from TargetDistributionDataCheck as floats rather string #3085

Merged
merged 6 commits into from
Nov 22, 2021

Conversation

angela97lin
Copy link
Contributor

Closes #2943. I chose not to keep the string representation and moved the calculations into a separate helper method, but open to discussion if we feel like the string representation is helpful in addition to the float values.

@angela97lin angela97lin self-assigned this Nov 19, 2021
@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #3085 (4db17e3) into main (3831e33) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3085     +/-   ##
=======================================
+ Coverage   99.8%   99.8%   +0.1%     
=======================================
  Files        313     313             
  Lines      30468   30470      +2     
=======================================
+ Hits       30378   30380      +2     
  Misses        90      90             
Impacted Files Coverage Δ
...hecks_tests/test_target_distribution_data_check.py 100.0% <ø> (ø)
...alml/data_checks/target_distribution_data_check.py 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3831e33...4db17e3. Read the comment docs.

@@ -142,3 +126,26 @@ def validate(self, X, y):
)

return results


def _detect_log_distribution_helper(y):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving mathy-implementation chunk into helper method to make validate easier to read!

Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

very nice!

@angela97lin angela97lin merged commit 2772536 into main Nov 22, 2021
@angela97lin angela97lin deleted the 2943_update_details branch November 22, 2021 16:59
@chukarsten chukarsten mentioned this pull request Nov 29, 2021
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.

Return details from TargetDistributionDataCheck as floats rather than / in addition to string
2 participants