Categorical Stop Condition Options #808

micdavis · 2023-05-03T19:30:06Z

No description provided.

…stop_condition to CategoricalOptions

JGSweets · 2023-05-03T20:58:38Z

dataprofiler/profilers/profiler_options.py

+        max_sample_size_to_check_stop_condition: int = None,
+        stop_condition_unique_value_ratio: float = None,


The typing should have the | None
max_sample_size_to_check_stop_condition: int | None = None
same for both.

dataprofiler/profilers/profiler_options.py

JGSweets · 2023-05-03T21:02:31Z

dataprofiler/tests/profilers/profiler_options/test_categorical_options.py

@@ -11,15 +11,66 @@ class TestCategoricalOptions(TestBaseInspectorOptions):
    def test_init(self):
        option = self.get_options()


Another PR, but this is naming I should have caught before:
get_options -> set_and_retrieve_option_values or something. Needs to be an issue.

get_options is confusing when we set a value in it.

JGSweets · 2023-05-03T21:04:44Z

dataprofiler/tests/profilers/profiler_options/test_categorical_options.py

@@ -32,6 +83,8 @@ def test_set(self):
        params_to_check = [
            dict(prop="is_enabled", value_list=[False, True]),
            dict(prop="top_k_categories", value_list=[None, 3]),
+            dict(prop="max_sample_size_to_check_stop_condition", value_list=[None, 20]),
+            dict(prop="stop_condition_unique_value_ratio", value_list=[None, 2]),


i suggest adding a float here

JGSweets · 2023-05-03T21:06:20Z

dataprofiler/tests/profilers/profiler_options/test_categorical_options.py

+            [expected_max_sample_size_type_error], options._validate_helper()
+        )
+
+        # These ones should throw a max_sample_size_type_error


comment should be for unique value I think

JGSweets · 2023-05-03T21:07:34Z

dataprofiler/tests/profilers/profiler_options/test_categorical_options.py

+        options = self.get_options(
+            max_sample_size_to_check_stop_condition=0,
+            stop_condition_unique_value_ratio=0.0,
+        )


so the condition we discussed before, it might be good to make one of these a value. and one 0 to test that condition.

could do this as
stop_condition_unique_value_ratio=0.5,

and in check above
set stop_condition_unique_value_ratio=0.0

so we don't need more tests to do the same checks

JGSweets · 2023-05-03T21:08:30Z

dataprofiler/profilers/profiler_options.py

+        if self.stop_condition_unique_value_ratio is not None and (
+            not isinstance(self.stop_condition_unique_value_ratio, float)
+            or self.stop_condition_unique_value_ratio < 0
+        ):


Because it is a ratio we need to: 0 <= stop_condition_unique_value_ratio <= 1.0

ah I knew it didn't makes sense for a hard core < 0

dataprofiler/tests/profilers/profiler_options/test_categorical_options.py

taylorfturner · 2023-05-03T21:08:55Z

dataprofiler/tests/profilers/profiler_options/test_categorical_options.py

@@ -32,6 +83,8 @@ def test_set(self):
        params_to_check = [
            dict(prop="is_enabled", value_list=[False, True]),
            dict(prop="top_k_categories", value_list=[None, 3]),
+            dict(prop="max_sample_size_to_check_stop_condition", value_list=[None, 20]),
+            dict(prop="stop_condition_unique_value_ratio", value_list=[None, 2]),


…stop_condition to CategoricalOptions (#808)

* [WIP] Part 1 fix for categorical mem opt issue (#795) * part_1 of fix for mem optimization for categoical dict creation issue * precommit fix * Separated the update from the check in stop conditions for categoical columns * added tests and accounted for different varaibles affected by the change made to categories attribute * Modifications to code based on test findings * Fixes for logic and tests to match requirements from PR * Fix for rebase carry over issue * fixes for tests because of changes to variable names in categorical column object * precommit fixes and improvement of code based on testing * added stop_condition_unique_value_ratio and max_sample_size_to_check_stop_condition to CategoricalOptions (#808) * implementation of setting stop conds via options for cat column profiler (#810) * Space time analysis improvement (#809) * Made space time analysis code improvements (detect if dataset is already generated, specify cats to generate) * Modified md file to account for new variable in space time analysis code * fix: cat bug (#816) * hotfix for more conservatitive stop condition in categorical columns (#817) * [WIP] Fix for histogram merging (#815) * rough draft of merge fix for histograms * final fixes for passing of existing tests * Added option to remove calculations for updating row statistics (#827) * Fix to doc strings (#829) * Preset Option Fix: presets docsstring added (#830) * presets docsstring added * Update dataprofiler/profilers/profiler_options.py * Update dataprofiler/profilers/profiler_options.py Co-authored-by: Taylor Turner <taylorfturner@gmail.com> * Update dataprofiler/profilers/profiler_options.py * Update dataprofiler/profilers/profiler_options.py --------- Co-authored-by: Taylor Turner <taylorfturner@gmail.com> --------- Co-authored-by: ksneab7 <91956551+ksneab7@users.noreply.github.com> Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com> Co-authored-by: JGSweets <JGSweets@users.noreply.github.com>

added stop_condition_unique_value_ratio and max_sample_size_to_check_…

10a8912

…stop_condition to CategoricalOptions

micdavis requested review from JGSweets, ksneab7, taylorfturner and tyfarnan as code owners May 3, 2023 19:30

taylorfturner assigned micdavis May 3, 2023

taylorfturner added the Work In Progress Solution is being developed label May 3, 2023

taylorfturner enabled auto-merge (squash) May 3, 2023 19:32

micdavis added 2 commits May 3, 2023 15:36

fixed conditional

ce0b990

added tests to cover functionality

8f6b428

taylorfturner removed the Work In Progress Solution is being developed label May 3, 2023

JGSweets reviewed May 3, 2023

View reviewed changes

fixed typing

dfe278b

taylorfturner reviewed May 3, 2023

View reviewed changes

JGSweets reviewed May 3, 2023

View reviewed changes

taylorfturner approved these changes May 3, 2023

View reviewed changes

JGSweets reviewed May 3, 2023

View reviewed changes

taylorfturner requested changes May 3, 2023

View reviewed changes

added more test cases

3172856

taylorfturner previously approved these changes May 3, 2023

View reviewed changes

comment fix

d7e6fe8

micdavis dismissed taylorfturner’s stale review via d7e6fe8 May 3, 2023 21:24

taylorfturner approved these changes May 3, 2023

View reviewed changes

JGSweets approved these changes May 3, 2023

View reviewed changes

taylorfturner merged commit 8c0cf87 into capitalone:feature/memory-optimization May 3, 2023
5 checks passed

ksneab7 pushed a commit that referenced this pull request May 23, 2023

added stop_condition_unique_value_ratio and max_sample_size_to_check_…

cca1dd0

…stop_condition to CategoricalOptions (#808)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Categorical Stop Condition Options #808

Categorical Stop Condition Options #808

micdavis commented May 3, 2023

JGSweets May 3, 2023

micdavis May 3, 2023

JGSweets May 3, 2023

JGSweets May 3, 2023

taylorfturner May 3, 2023

JGSweets May 3, 2023

taylorfturner May 3, 2023

JGSweets May 3, 2023

JGSweets May 3, 2023

JGSweets May 3, 2023

JGSweets May 3, 2023

taylorfturner May 3, 2023

taylorfturner May 3, 2023

		max_sample_size_to_check_stop_condition: int = None,
		stop_condition_unique_value_ratio: float = None,

		@@ -11,15 +11,66 @@ class TestCategoricalOptions(TestBaseInspectorOptions):
		def test_init(self):
		option = self.get_options()

Categorical Stop Condition Options #808

Categorical Stop Condition Options #808

Conversation

micdavis commented May 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment