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

Add WordMatch + WordNoMatch descriptor #1334

Merged
merged 20 commits into from
Oct 17, 2024

Conversation

jon-bown
Copy link
Contributor

@jon-bown jon-bown commented Oct 5, 2024

Implement WordMatch, WordNoMatch, descriptor + feature and associated unit tests.

Resolves issue #1308
Resolves issue #1309

@jon-bown jon-bown changed the title Add WordMatch descriptor Add WordMatch + WordNoMatch descriptor Oct 5, 2024
@emeli-dral
Copy link
Contributor

Hi @jon-bown ,
This one looks good as well!

Could you please double check your src/evidently/features/_registry.py?
It looks like paths to feature classes are invalid.

@jon-bown
Copy link
Contributor Author

jon-bown commented Oct 7, 2024

Hi @jon-bown , This one looks good as well!

Could you please double check your src/evidently/features/_registry.py? It looks like paths to feature classes are invalid.

Done! Also want to point out that the error you're getting with my ItemMatch PR would need to be resolved here also.

@emeli-dral
Copy link
Contributor

Done! Also want to point out that the error you're getting with my ItemMatch PR would need to be resolved here also.

Thank you for the update, fair enough! I suggest you change a list to a tuple here as well, similar to what we’ve discussed in the related issue. This should help prevent the error and maintain consistency across both implementations.

Thanks for your attention to detail! 🙏

@emeli-dral emeli-dral added the hacktoberfest Accepted contributions will count towards your hacktoberfest PRs label Oct 14, 2024
@emeli-dral
Copy link
Contributor

Hi @jon-bown ,
It looks like there’s a small issue with the Python f-string here:

File "/home/runner/work/evidently/evidently/src/evidently/features/words_feature.py", line 198
    default_display_name=f"Text contains {self.mode.split("_")[1]} defined words"

I believe the string is being parsed incorrectly due to the nested double quotes inside the f-string. To resolve this, try changing the internal double quotes to single quotes ('_').

That should fix the issue!

@jon-bown
Copy link
Contributor Author

Hi @emeli-dral I believe I've fixed all the issues with this PR. Can you let me know if anything else needs to change? Thanks!

@emeli-dral
Copy link
Contributor

Hi @jon-bown,
I had a chance to review your repo, and I really like how the descriptor is working—great job! However, I would like to request one more small change, apologies for the extra ask!

To keep things consistent and avoid potential confusion for users, I’d suggest making the WordMatch descriptor work directly with tuples, just like the ItemMatch descriptor. Having one descriptor use tuples and another use lists might be a bit unclear.

In the future, we plan to support lists as an input type in a more centralized way, and we’ll introduce list support across all relevant descriptors then.

Thanks again for your hard work!

@jon-bown
Copy link
Contributor Author

Hi @emeli-dral, thanks for the update! As far as I can see, this implementation matches the ItemMatch/NoMatch implementation. Is there anything in particular you can see that needs to change? In the unit tests I convert the lists to tuples before they are fed to the feature, same as the other PR so it won't work with lists inside the data frame. I will also resolve the merge conflicts in the meantime.

@emeli-dral
Copy link
Contributor

Hi @jon-bown,
You’re absolutely right, that was my mistake — thank you for pointing that out! I've merged main into your branch, and I'll proceed with the merge as soon as the tests pass.

Thanks again for your patience and your thorough work on this!

@emeli-dral emeli-dral merged commit 0d02c62 into evidentlyai:main Oct 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Accepted contributions will count towards your hacktoberfest PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants