HistogramOptions now named to HistogramAndQuantilesOptions, implement num_quantiles option to HistogramAndQuantilesOptions
#956
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
clee1152
requested review from
JGSweets,
ksneab7,
taylorfturner,
micdavis and
tyfarnan
as code owners
JGSweets
reviewed
Jul 12, 2023
JGSweets
reviewed
Jul 12, 2023
JGSweets
previously requested changes
Jul 12, 2023
?? |
taylorfturner
changed the base branch from
feature/quantile_options
to
feature/num-quantiles
…es_option.py Co-authored-by: Taylor Turner <[email protected]>
auto-merge was automatically disabled
July 13, 2023 18:48
Head branch was pushed to by a user without write access
|
Going to create a new PR for adding deprecated |
auto-merge was automatically disabled
July 19, 2023 17:12
Head branch was pushed to by a user without write access
auto-merge was automatically disabled
July 20, 2023 14:28
Head branch was pushed to by a user without write access
taylorfturner
suggested changes
Jul 20, 2023
dataprofiler/tests/profilers/profiler_options/test_histogram_and_quantiles_option.py
Outdated
Show resolved
Hide resolved
…nd_quantiles_option.py Co-authored-by: Taylor Turner <[email protected]>
auto-merge was automatically disabled
July 20, 2023 17:38
Head branch was pushed to by a user without write access
auto-merge was automatically disabled
July 20, 2023 17:40
Head branch was pushed to by a user without write access
Comment on lines
116
to
119
| self.quantiles: list[float] | None = None | ||
| self.quantiles: list[float] | dict = { | ||
| bin_num: None for bin_num in range(self._num_quantiles - 1) | ||
| } |
taylorfturner
suggested changes
Jul 20, 2023
dataprofiler/tests/profilers/test_numeric_stats_mixin_profile.py
Outdated
Show resolved
Hide resolved
auto-merge was automatically disabled
July 20, 2023 19:25
Head branch was pushed to by a user without write access
| @@ -71,13 +81,58 @@ def _assert_set_helper(prop, value): | |||
| with self.assertRaisesRegex(AttributeError, expected_error): | |||
| option.set({"bin_count_or_method.is_enabled": True}) | |||
|
|
|||
| # Treat num_quantiles as a BooleanOption | |||
| expected_error = "type object 'num_quantiles' has no attribute " "'is_enabled'" | |||
taylorfturner
suggested changes
Jul 20, 2023
Comment on lines
1841
to
1844
| int_options = FloatOptions() | ||
| int_options.histogram_and_quantiles.bin_count_or_method = 5 | ||
| int_options.histogram_and_quantiles.num_quantiles = 4 | ||
| profiler = FloatColumn("0.0", int_options) |
There was a problem hiding this comment.
😢 bad naming.... but we should fix in this PR
taylorfturner
approved these changes
Jul 20, 2023
Comment on lines
+1841
to
+1844
| float_options = FloatOptions() | ||
| float_options.histogram_and_quantiles.bin_count_or_method = 5 | ||
| float_options.histogram_and_quantiles.num_quantiles = 4 | ||
| profiler = FloatColumn("0.0", float_options) |
JGSweets
approved these changes
Jul 20, 2023
taylorfturner
added a commit
that referenced
this pull request
Aug 1, 2023
…nt `num_quantiles` option to `HistogramAndQuantilesOptions` (#956) * Implemented options for `num_quantiles` feature in `NumericStatsMixin` * Implemented options for `num_quantiles.py`. * Added tests for `num_quantiles`. * Fixed tests for num_quantiles option in column profiles. * Added tests for num_quantiles option. * Update dataprofiler/tests/profilers/profiler_options/test_num_quantiles_option.py Co-authored-by: Taylor Turner <[email protected]> * Minor edits to tests * Empty commit * Precommits * Minor change to Makefile. * Got rid of print statements. * "empty commit" * Added deprecation for `HistogramOption`. * Cleaner deprecation: removed `HistogramOption` class. * json_decoder, test_h_a_q changes. * Fixed string bug for testing in test_histogram_and_quantiles * Explicitfied deprecation warning, validated profile conversion from histogramoption to histogramandquantilesoption in decoding phase. * Minor deprecation warning wording change. * Update dataprofiler/tests/profilers/profiler_options/test_histogram_and_quantiles_option.py Co-authored-by: Taylor Turner <[email protected]> * Changed deprecation warning. * Changed quantiles option to initialize to None. * Minor change * Minor change again D: * Minor change again again D: sadness --------- Co-authored-by: Taylor Turner <[email protected]>
clee1152
added a commit
to clee1152/DataProfiler
that referenced
this pull request
Aug 1, 2023
…nt `num_quantiles` option to `HistogramAndQuantilesOptions` (capitalone#956) * Implemented options for `num_quantiles` feature in `NumericStatsMixin` * Implemented options for `num_quantiles.py`. * Added tests for `num_quantiles`. * Fixed tests for num_quantiles option in column profiles. * Added tests for num_quantiles option. * Update dataprofiler/tests/profilers/profiler_options/test_num_quantiles_option.py Co-authored-by: Taylor Turner <[email protected]> * Minor edits to tests * Empty commit * Precommits * Minor change to Makefile. * Got rid of print statements. * "empty commit" * Added deprecation for `HistogramOption`. * Cleaner deprecation: removed `HistogramOption` class. * json_decoder, test_h_a_q changes. * Fixed string bug for testing in test_histogram_and_quantiles * Explicitfied deprecation warning, validated profile conversion from histogramoption to histogramandquantilesoption in decoding phase. * Minor deprecation warning wording change. * Update dataprofiler/tests/profilers/profiler_options/test_histogram_and_quantiles_option.py Co-authored-by: Taylor Turner <[email protected]> * Changed deprecation warning. * Changed quantiles option to initialize to None. * Minor change * Minor change again D: * Minor change again again D: sadness --------- Co-authored-by: Taylor Turner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR changes:
HistogramOptiontoHistogramAndQuantilesOptionHistogramAndQuantilesOptionnow contains new attributenum_quantilesHistogramOptionnow deprecated, loadsHistogramAndQuantilesOptioninjson_decoderFloatColumnProfile,TextColumnProfile,IntColumnProfile,NumericStatsMixinProfile, andHistogramAndQuantilesOption