-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Support for top-k value matches #80
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
eff84d8
to
453a436
Compare
930b65f
to
e60f14c
Compare
f7048cd
to
1fcfe8f
Compare
1fcfe8f
to
532b8d1
Compare
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.
I'm still taking a look at this in detail, but I think we need to add unit tests for the top_value_matches()
API.
I overlooked adding the test but will do so shortly, thanks! |
c917303
to
b6386b6
Compare
BREAKING CHANGE: this change may change the behavior of code that relies on edit being equal to True.
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.
@roquelopez I made a few minor changes and commited the notebook/code. Feel free to merge when you review.
if "top_n" in method_args and method_args["top_n"] > 1: | ||
logger.warning( | ||
f"Ignoring 'top_n' argument, use the 'top_value_matches()' method to get top-k value matches." | ||
) | ||
method_args["top_n"] = 1 |
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.
Instead of top_n
, what about we use top_k
to be consistent with other function parameters?
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 , thanks!
It is related to issue #61.