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
[BUGFIX] Add new high precision rule for mean and stdev in OnboardingDataAssistant
#5276
[BUGFIX] Add new high precision rule for mean and stdev in OnboardingDataAssistant
#5276
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
👇 Click on the image for a new way to code review
Legend |
great_expectations/rule_based_profiler/data_assistant/onboarding_data_assistant.py
Show resolved
Hide resolved
…tandard-deviation-rounding-error
…tandard-deviation-rounding-error
great_expectations/rule_based_profiler/data_assistant/onboarding_data_assistant.py
Outdated
Show resolved
Hide resolved
great_expectations/rule_based_profiler/data_assistant/onboarding_data_assistant.py
Show resolved
Hide resolved
great_expectations/rule_based_profiler/data_assistant/onboarding_data_assistant.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NathanFarmer We also need to update the V3 Profile Jupyter Notebook renderer in a way that changes the rule names in the stock example for how to configure the variables. For example, you can copy the existing rule settings twice (with the appropriate name changes) and leaving your new tolerances equal to your new low/high precision defaults. The corresponding tests as well as the notebook(s) created by @Shinnnyshinshin will also need to be updated (this is a simple copy/paste from the above). Happy to discuss. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NathanFarmer All good regarding rule naming and implementations -- just need to take care of the renderer (you can find which renderer by searching for the OnboardingDataAssistantResut
in the repository (including tests
). Thanks!
P.S.: Please do not forget about any applicable tests/integration
and docs
notebooks. Thanks!
…ion-rounding-error' of github.com:great-expectations/great_expectations into bugfix/OnboardingDataAssistant-mean-and-standard-deviation-rounding-error
…tandard-deviation-rounding-error
…tandard-deviation-rounding-error
great_expectations/render/renderer/v3/suite_profile_notebook_renderer.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NathanFarmer A couple of "consistency" requests -- almost there! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NathanFarmer saw tiny inconsistency regarding round_decimals
in two places. Almost! Thanks!
…eviation-rounding-error
…ion-rounding-error' of github.com:great-expectations/great_expectations into bugfix/OnboardingDataAssistant-mean-and-standard-deviation-rounding-error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!! Yay!
Changes proposed in this pull request:
Before (expectation fails approximately 2/3 of the time):
After (expectation fails at approximately the
false_positive_rate
of 0.05):Definition of Done