Stop calculating top_values for Double columns with integer values
#1692
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
tamargrey
commented
May 1, 2023
| @@ -1369,7 +1369,8 @@ def test_describe_callback(describe_df, mock_callback): | |||
| assert mock_callback.total_elapsed_time > 0 | |||
|
|
|||
|
|
|||
| def test_describe_dict_extra_stats(describe_df): | |||
| @pytest.mark.parametrize("use_age", [True, False]) | |||
| def test_describe_dict_extra_stats(use_age, describe_df): | |||
There was a problem hiding this comment.
I wanted to make sure to cover age usage and confirm its behavior matches int and double usage, but I recognize that this may be a bit overkill. If anyone would rather I make a completely separate test that more minimally tests age vs integer usage, lmk.
Codecov Report
@@ Coverage Diff @@
## main #1692 +/- ##
=======================================
Coverage 98.79% 98.79%
=======================================
Files 98 98
Lines 11805 11809 +4
=======================================
+ Hits 11663 11667 +4
Misses 142 142
|
bchen1116
approved these changes
May 2, 2023
tamargrey
force-pushed
the
remove-double-top-values
branch
from
49c7da5
to
25f3778
Compare
thehomebrewnerd
approved these changes
May 2, 2023
Comment on lines
+177
to
+180
| if isinstance( | ||
| column.logical_type, | ||
| (Age, AgeNullable, Integer, IntegerNullable), | ||
| ): |
There was a problem hiding this comment.
Not something we need in this PR, but might be nice if we had an is_integer_type property on the column schema to use instead of having to maintain a list of logical types here.
Merged
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.
_get_describe_dict