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

Improve Genius title matching #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve Genius title matching #36

wants to merge 3 commits into from

Conversation

aadibajpai
Copy link
Member

issues in mind:

  • current comparison checks for list elements in a string, the disadvantage of this is that partial matches will return true, as in 'na' in 'banana' or 'sass' in 'assassination' will return true but with list comparison, 'na' in ['banana'] will return false.

  • one sort of false positives is when a different song of the same artist is returned. eg.
    image
    this can be fixed by checking the song name specifically but then

  • this creates a problem with how the discord bot works, people are supposed to specify song artist like "rap god" "eminem" but no one bothers to read help so they just do rap god eminem, which gets treated as song=rap, artist=god, the bot still manages to return lyrics because of lax matching.

  • for remixes, song name will remain same but artist might be referring to the remix artist which would again trip up the matcher if artist is specifically compared.

Signed-off-by: Aadi Bajpai [email protected]

Signed-off-by: Aadi Bajpai <[email protected]>
@aadibajpai aadibajpai requested a review from flabbet July 22, 2020 13:20
@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #36 into master will increase coverage by 0.79%.
The diff coverage is 85.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   82.33%   83.13%   +0.79%     
==========================================
  Files           3        3              
  Lines         419      581     +162     
  Branches       56       96      +40     
==========================================
+ Hits          345      483     +138     
- Misses         73       94      +21     
- Partials        1        4       +3     
Impacted Files Coverage Δ
swaglyrics_backend/issue_maker.py 85.53% <85.00%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71b8c4c...d253a1c. Read the comment docs.

Aadi Bajpai added 2 commits July 24, 2020 11:49
Signed-off-by: Aadi Bajpai <[email protected]>
Signed-off-by: Aadi Bajpai <[email protected]>
@aadibajpai aadibajpai marked this pull request as ready for review July 29, 2020 23:09
@aadibajpai
Copy link
Member Author

@thealphadollar, tested with actual data, splitting before checking seems to work.

Copy link

@thealphadollar thealphadollar left a comment

Choose a reason for hiding this comment

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

A small suggestion, rest looks good to me.

Have you thought about solving the challenges you've mentioned in the description?

@aadibajpai
Copy link
Member Author

Have you thought about solving the challenges you've mentioned in the description?

this solves those! the split() takes care of the first one and the second and third one are addressed when the artist is checked and the fourth one is for the discord cases.

@thealphadollar
Copy link

Have you thought about solving the challenges you've mentioned in the description?

this solves those! the split() takes care of the first one and the second and third one are addressed when the artist is checked and the fourth one is for the discord cases.

That's pretty cool. I got primarily confused with the description since it mentioned only the issues in detail. Good to go!

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

Successfully merging this pull request may close these issues.

2 participants