-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: make to_pandas override enable_downsampling when sampling_method is manually set. #200
Conversation
… is manually set.
bigframes/core/blocks.py
Outdated
@@ -454,6 +437,29 @@ def _compute_and_count( | |||
) -> Tuple[pd.DataFrame, int, bigquery.QueryJob]: | |||
"""Run query and download results as a pandas DataFrame. Return the total number of results as well.""" | |||
# TODO(swast): Allow for dry run and timeout. | |||
enable_downsampling = bigframes.options.sampling.enable_downsampling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put code together for the same logic?
For example,
enable_downsampling = sampling_method is not None or bigframes.options.sampling.enable_downsampling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if random_state is None: | ||
random_state = bigframes.options.sampling.random_state | ||
|
||
sampling_method = sampling_method.lower() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put sampling_method logic together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
… is manually set.
else bigframes.options.sampling.enable_downsampling | ||
) | ||
|
||
max_download_size = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: less code:
max_download_size = max_download_size or bigframes.options.sampling.max_download_size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
… is manually set.
… is manually set. (#200) * fix: make to_pandas override enable_downsampling when sampling_method is manually set. * fix: make to_pandas override enable_downsampling when sampling_method is manually set. * fix: make to_pandas override enable_downsampling when sampling_method is manually set.
… is manually set.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes b/310741105