Part 1 fix for categorical mem opt issue

[ksneab7]

Dataset used is a single column ordered dataset
Conditions set:
self.max_sample_size_to_check_stop_condition = 100
self.stop_condition_unique_value_ratio = .90

• 0
  • Profile
    • Pre change
      • Total space allocated: N/A
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.02222418785095215
    • Post change
      • Total space allocated: N/A
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.022262096405029297
  • Merge
    • Pre change
      • Total space allocated: N/A
      • Line specific space allocated (Line number/s):N/A
      • Merge runtime: 1.4066696166992188e-05
    • Post change
      • Total space allocated: N/A
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 1.0967254638671875e-05
• 100
  • Profile
    • Pre change
      • Total space allocated: 449.9 KiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.11128807067871094
    • Post change
      • Total space allocated: 449.9 KiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.10366582870483398
  • Merge
    • Pre change
      • Total space allocated: 160.8 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.02127695083618164
    • Post change
      • Total space allocated: 160.8 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.02112579345703125
• 1000
  • Profile
    • Pre change
      • Total space allocated: 269.5 KiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.07865023612976074
    • Post change
      • Total space allocated: 269.5 KiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.07574105262756348
  • Merge
    • Pre change
      • Total space allocated: 220.8 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.025424957275390625
    • Post change
      • Total space allocated: 220.8 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.022446870803833008
• 5000
  • Profile
    • Pre change
      • Total space allocated: 841.1 KiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.141251802444458
    • Post change
      • Total space allocated: 841.1 KiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.13668394088745117
  • Merge
    • Pre change
      • Total space allocated: 653.0 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.03322911262512207
    • Post change
      • Total space allocated: 653.0 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.03174471855163574
• 7500
  • Profile
    • Pre change
      • Total space allocated: 1.3 MiB
      • Line specific space allocated (Line number/s): 288.0 KiB
      • Profile runtime: 0.18341469764709473
    • Post change
      • Total space allocated: 1.2 MiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 0.1740732192993164
  • Merge
    • Pre change
      • Total space allocated: 1002.1 KiB
      • Line specific space allocated (Line number/s):
      • Merge runtime: 0.03673124313354492
    • Post change
      • Total space allocated: 1002.1 KiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.034219980239868164
• 100000
  • Profile
    • Pre change
      • Total space allocated: 22.6 MiB
      • Line specific space allocated (Line number/s): 5.0 MiB
      • Profile runtime: 1.42696213722229
    • Post change
      • Total space allocated: 17.6 MiB
      • Line specific space allocated (Line number/s): N/A
      • Profile runtime: 1.4447557926177979
  • Merge
    • Pre change
      • Total space allocated: 13.0 MiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.21359801292419434
    • Post change
      • Total space allocated: 13.0 MiB
      • Line specific space allocated (Line number/s): N/A
      • Merge runtime: 0.15265393257141113

Separate measure for two different profiles to indicate merge differences:

 - Pre change
    - Total space allocated: 25.0 MiB
    - Line specific space allocated (Line number/s): N/A
- Post change
    - Total space allocated: 17.7 MiB
    - Line specific space allocated (Line number/s): N/A

[JGSweets]

merged_unique_count = len(self._categories)
merged_sample_size = (self.sample_size + len(data))
merged_unique_ratio =  merged_unique_count / merged_sample_size


if (
    self.max_sample_size_to_check_stop_condition is not None
    and  self. stop_condition_unique_value_ratio is not None
    and merged_sample_size >= self.max_sample_size_to_check_stop_condition
    and merged_unique_ratio >= stop_condition_unique_value_ratio
):
    self._stop_condition_is_met = True
    self._stopped_at_unique_ratio = merged_unique_ratio
    self._stopped_at_unique_count = merged_unique_count

I think this keeps it clear and then sets the values.

An option would be if we want this function to just be a check, we can calculate in_update_categories :
merged_unique_count = len(self._categories)
merged_sample_size = (self.sample_size + len(data))
merged_unique_ratio = merged_unique_count / merged_sample_size

and pass it into the check function:
```python
def _check_stop_condition_is_met(self, sample_count, sample_size, unqiue_ratio):
    """Return value stop_condition_is_met given stop conditions.
    
    :return: boolean for stop conditions
    """
    if (
          self.max_sample_size_to_check_stop_condition is not None
          and  self. stop_condition_unique_value_ratio is not None
          and sample_size >= self.max_sample_size_to_check_stop_condition
          and unique_ratio >= stop_condition_unique_value_ratio
    ):
        return True
    return False

then in _update_categories:

if self._check_stop_condition_is_met(merged.....):
    self._stop_condition_is_met = True
    self._categories = {}
    self._stopped_at_unique_ratio = merged_unique_ratio
    self._stopped_at_unique_count = merged_unique_count

^^ similar logic can then be used in the __add__ where we calc the:

if not profile1._stop_condition_is_met and not profile2._stop_condition_is_met:
    # merge dicts and then check the values for the merging with the stop condition, etc.
else:
    # merged categories is empty
    # unique ratio and unique count can be take from the profile with the largest sample size
    # if we avg, will be problem for streams.
 
 
# also transfer stop conditions: enforce the most expensive /lenient case for now (open to either case)

between the two profies

[JGSweets]

as above, default should suffice in this case.

[ksneab7]

same comment as above

[JGSweets]

if we already set:

self._stopped_at_unique_ratio = merged_unique_ratio
self._stopped_at_unique_count = merged_unique_count

below, we can join it with:

self._categories = {}

However, let's consider the function description and the action of the function.

When we say update_stop_condition, to me it would only update _stop_condition_is_met boolean. Do we wnat to rename it to be more explicit about it doing more? or do we need update_stop_condition?

Could we just have _update_categories and _check_stop_condition_is_met?

[JGSweets]

Want to note these are code readability / quality comments, not functional as I think the code is in a functional state.

[JGSweets]

Since this is a feature branch, we can refactor in subsequent PR.

[JGSweets]

if we want this to continue to exist, I suggest making it private as we wouldn't want an external user to use this method.

[ksneab7]

why wouldnt we want that?

[JGSweets]

Should it be purely based on max?

Consider:

• Profile 1 has 10 samples and a unique_ratio of 1.00
• Profile 2 has 10000 samples and a unique ratio of 0.50.

Do these values make sense?

Also, do we want to potentially choose different values from each, could that be problematic?

[JGSweets]

The option of not choosing one profiles values vs the other's complicates the sample_size below I believe.

[JGSweets]

We should also have tests which ensure that if self has the values that get assigned vs swapping and ensuring that other's values can be chosen.

[ksneab7]

We need to talk about this more thoroughly, I feel that not matter what we choose here, in the current state, there is going to be a problem and there isnt an alternative suggested here, so not sure how to move forward.

[JGSweets]

We shouldn't later merged_profile times. This should already be handled via the _add_helper

[JGSweets]

we should make sure our tests validates this.

[ksneab7]

No we should not add the times together because that is not consistent with a stop condition being hit

[JGSweets]

I don't believe this is true if we are taking one value over the other above

[ksneab7]

I dont agree with this statement (mentioned above)

[JGSweets]

should ensure times are correct, but looks like we don't properly do that in this file rn.

The way we've done previously is mocking timeit as a generator.
Example:

DataProfiler/dataprofiler/tests/profilers/test_profile_builder.py

Line 106 in f206af2

with test_utils.mock_timeit():

[JGSweets]

Should add issue for this

[JGSweets]

added:
#806

[JGSweets]

Concerns about merge and suggestions for the update portion.

[JGSweets]

This still needs to be conducted above. right now we don't have it populated, but it should be done in either case.

[JGSweets]

Assuming refactors after this in subsequent PRs

[taylorfturner]

Assuming refactors after this in subsequent PRs

need these refactors to be notated as follow-up issues @ksneab7