Skip to content
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

[cmdpalette] fix scoring of space-separated search terms #2646

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

midichef
Copy link
Contributor

@midichef midichef commented Dec 20, 2024

Fixes part of #2645.

When multiple search terms are used in fuzzymatch(), not all the terms are used for a match. Only the last matching term is used to score each part of the match. That is, for longname, only the last matching term's score is used, and for description, only the last matching term's score is used.

To see this, hit Space, then type select a. select-after should be the top result, but it's not shown at all. (The proper rankings should look more like what you see searching for selecta with no space.)

That's because select-after is incorrectly scored as a match for a only, not as a match for select and a match for a. So it is incorrectly pushed way down the ranking. That leaves select-error-col as the top result, because it does not have any match for a dragging it down. It gets the highest score by matching solely for select and not for a.

This PR lets each term contribute to a combined score. With it, select a properly ranks select-after and select-around-n as the top two results. (There are remaining differences in the rankings with selecta vs select a, because the select a search adds a bonus to results where description has a at the start of a word instead of just in the middle.)

@aborruso
Copy link
Contributor

Thank you very much

Copy link
Owner

@saulpw saulpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, @midichef !

@aborruso
Copy link
Contributor

Hi @midichef what should I do if I want to use it?

Do I must install the develop version running pip3 install git+https://github.com/saulpw/visidata.git@develop?

Thak you to you, @saulpw and @anjakefala

Previously only the score for the final search term was used.
@midichef midichef force-pushed the fuzzymatch_with_spaces branch from 09567e1 to 264387d Compare December 21, 2024 10:30
@midichef
Copy link
Contributor Author

I pushed a new commit that didn't change any code, only the description, to insert a - into [cmdpalette].

@midichef
Copy link
Contributor Author

midichef commented Dec 24, 2024

@aborruso yes, once the PR is accepted into the develop branch, you can use that command, or the shorter form pip3 install git+https://github.com/saulpw/visidata.git. But the current develop has a separate bug that slows down the loading of large files, which is not present in vd v3.1.1. That bug will be fixed by #2636, so I suggest you wait till that is in develop too.

Do I must install the develop version running pip3 install git+https://github.com/saulpw/visidata.git@develop?

@midichef
Copy link
Contributor Author

@aborruso
Until both those fixes are up, I merged both those fixes into a branch on my fork of visidata. You can install it with: pip3 install git+https://github.com/midichef/[email protected]_space_and_max_rows
Feel free to try it if you'd like. I'll keep it available until both the fixes have made it into develop, then I'll take it down.

@aborruso
Copy link
Contributor

Thank you @midichef a stupid question: is it better first to uninstall the version I have and then install the new?

@midichef
Copy link
Contributor Author

I uninstall first. But I don't actually know if that is better.

@aborruso
Copy link
Contributor

@midichef wow, it works.

I had noticed this for a very long time. I had never written it down, because it seemed strange to me that no one had ever noticed it, and then I thought there must be some reason why it must be malfunctioning :)

Thank you again

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants