Conversation
…nd `fraction_lists_to_search` parameters
| as the values in the ``column_to_search`` column. Can only be set when query is a DataFrame. | ||
| top_k (int, default 10): | ||
| top_k (int): | ||
| Sepecifies the number of nearest neighbors to return. Default to 10. |
There was a problem hiding this comment.
I thought it is a breaking change but it is not after digging into the BigQuery SQL document to confirm this. To ensure a smooth user experience, can we keep the default explicit to make the behavior immediately apparent without relying on external documentation? I am open to learning more your thoughts.
There was a problem hiding this comment.
user_brute_force is also applied for this comment. Thanks!
There was a problem hiding this comment.
Likewise, I want to keep BigFrames pretty lightweight. If users don't specify a value, I want to make sure we use the server-side default.
There was a problem hiding this comment.
I agree with this approach. It's important that the docstring clearly states the default value of 10, so users can easily understand the parameter's behavior without needing to consult server-side documentation. Because of that, I'm fine with the current implementation.
| """ | ||
| import bigframes.series | ||
|
|
||
| if not fraction_lists_to_search and use_brute_force is True: |
There was a problem hiding this comment.
I think we can keep this ValueError exception?
There was a problem hiding this comment.
Sudipto was encountering this error thinking it was a BigFrames bug. I'd rather remove the client-side check and let the server handle this case.
|
e2e failure |
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 internal issue 344019989 🦕