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

Feat/add contains link descriptor #1333

Merged
merged 10 commits into from
Oct 22, 2024

Conversation

Sifr-un
Copy link
Contributor

@Sifr-un Sifr-un commented Oct 4, 2024

Implementing "contains link" descriptor.
Resolves #1313

@emeli-dral
Copy link
Contributor

Hi @Rayryu,
Apologies for the delay in reviewing your PR. Thank you for your hard work—the implementation looks great! Could you please add the new descriptor to the list of descriptors under the TextEvals heading on the documentation page? Once that’s done, we’ll be ready to proceed with the merge.

Thanks again!

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

Sifr-un commented Oct 16, 2024

Hi @Rayryu, Apologies for the delay in reviewing your PR. Thank you for your hard work—the implementation looks great! Could you please add the new descriptor to the list of descriptors under the TextEvals heading on the documentation page? Once that’s done, we’ll be ready to proceed with the merge.

Thanks again!

No worries @emeli-dral ! I've added the descriptor under the TextEvals 👌

@emeli-dral
Copy link
Contributor

Hi @Rayryu,

Apologies for any confusion regarding the documentation. It looks like you've made changes to the TextEvals preset itself, which is a bit different. If we keep that change, this descriptor will be included in the preset by default, which isn’t quite what we're aiming for.

I was referring to the documentation only! The document is located here: all-metrics.md#text-evals.

Could I kindly ask you to revert the changes to the TextEvals preset itself and just add a line to the documentation?

Feel free to use the other descriptors as a reference! :)

@Sifr-un
Copy link
Contributor Author

Sifr-un commented Oct 21, 2024

Apologies for any confusion regarding the documentation. It looks like you've made changes to the TextEvals preset itself, which is a bit different. If we keep that change, this descriptor will be included in the preset by default, which isn’t quite what we're aiming for.

I was referring to the documentation only! The document is located here: all-metrics.md#text-evals.

Could I kindly ask you to revert the changes to the TextEvals preset itself and just add a line to the documentation?

Feel free to use the other descriptors as a reference! :)

Thanks for the clarification! I see what happened now. I'll revert the changes to the TextEvals preset and update the documentation as you suggested. Appreciate your patience and guidance on this!

@emeli-dral
Copy link
Contributor

Hi @Sifr-un ,

Thank you so much for your hard work on this PR! You've done an excellent job, and your efforts are really appreciated. The implementation looks great, and I’m happy to proceed with the merge.

Looking forward to seeing more of your contributions in the future!

@emeli-dral emeli-dral merged commit b61d96f into evidentlyai:main Oct 22, 2024
25 of 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.

Add a new ContainsLink descriptor to Evidently
2 participants