Make _assimilate_histogram() not use self #1071
Conversation
|
|
||
| # if it's not a float histogram, then assume it only contains integer values | ||
| if not is_float_histogram: | ||
| bin_edges = np.round(bin_edges) |
There was a problem hiding this comment.
this is a big assumption ... can you comment on the rationale here?
There was a problem hiding this comment.
The same assumption was being made in _assimilate_histogram (diff). I moved it from _assimilate_histogram to _regenerate_histogram.
We could also remove that rounding code entirely, but it causes a single test failure here. I haven't been able to figure out what purpose the rounding fulfills here, as it existed since the first release. I kept it so that the behavior of _regenerate_histogram wouldn't change from before.
But you're right that it's a big assumption, and it's worth considering its removal. I'll open an alternative PR for that
There was a problem hiding this comment.
PR #1073 is an alternative to this PR where we remove the rounding code
There was a problem hiding this comment.
Okay, after extensive discussion with @taylorfturner @micdavis we have determined that this assumption is valid and all values passed into this function at the current placement in the workflow will all be numeric (either ints and floats). I think there should be an additional test where this line 1389 is called (or if a test already exists) to validate that bin edges are able to be rounded at that point in the workflow (i.e. is it possible to get a non-roundable value at this point in the code, which I believe is not possible).
Assuming this test is created I am good with this version of code and we can delete PR 1073
There was a problem hiding this comment.
@ksneab7 Thanks for the feedback! Line 1389 is called in the existing test TestTextColumnProfiler.test_profile (I've attached a screenshot of my debugger stopping at line 1389 while running the test, to demonstrate this). Also, if this rounding code is removed entirely, the output changes and this test fails, as I explained in #1073.
Does this fulfill what you meant, or did you have something else in mind?
There was a problem hiding this comment.
Nope this is exactly what I was looking for thank you!
|
Taking a gander at these today -- thanks for the patience, @junholee6a! |
|
@junholee6a just one comment on this from @ksneab7. We opted for this PR over the alternative (closed now). Let us know if you have any questions. We are trying to release |
|
Thanks @junholee6a -- merged |
* Feature: added parquet sampling (#1070) * parquet sampling function developed in data_utils.py; Added sample_nrows argument in ParquetData class; Added test_len_sampled_data in test_parquet_data.py * resolved conflict with dev, added more tests * fixed sample empty column bug * fixed comments in data_utils.py, including: 1. added type of return in sample_parquet function; 2. changed variable names in sample_parquet function to more descriptive names (select -> sample_index, out -> sample_df); 3. created convert_unicode_col_to_utf8 function to reduce repeating code in sample_parquet and read_parquet_df functions * 1. renamed variable names in covert_unicode_col_to_utf8 function (data_utils.py) to be more descriptive (types -> input_column_types, col -> iter_column), other part unchanged 2. test_parquet_data.py, move import statement to the top of file 3. test_parquet_data.py, merged all tests about parquet sample feature to their original tests * checked the datatype and input file path before and after reload with sampling option enabled * test * delete test edit in avro_data.py, updated fastavro version in requirment.txt * remove fastavro.reader type * change fastavro version back to original * 1. sample_parquet function description 2. test_len_data method keep one sample length test 3. remove sampling test in test_specifying_data_type 4. remove sampling test in test_reload_data * Depedency: `matplotlib` version bump (#1072) * bump tag matplotlib * bumpt to most recent * 3.9.0 update * Bump actions/setup-python from 4 to 5 (#1078) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4 to 5. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v4...v5) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Turner <taylorfturner@gmail.com> * Make _assimilate_histogram not use self (#1071) Co-authored-by: Taylor Turner <taylorfturner@gmail.com> * version bump --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: WML <36968256+menglinw@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Junho Lee <53921230+junholee6a@users.noreply.github.com>
NOTE: This is an alternative of PR #1073. If this is merged, then close PR #1073
Issue: #820
This is a necessary step to resolving issue #820. Previously,
_assimilate_histogram()calledselfto decide whether the given histogram contained integers or floats, and rounded the bins for histograms that only contained integers._assimilate_histogram()is called in exactly two places:_regenerate_histogram()and_add_helper_merge_profile_histograms(). To remove the dependency onself, we can move the code for rounding bin edges to_regenerate_histograms()and add an argument indicating whether the given histogram contains integers or floats.While
_regenerate_histogram()should behave exactly the same as before, the histogram loss calculation in_add_helper_merge_profile_histograms()may change slightly since we no longer round its histogram bins.