Codecov Report

Merging #643 (096dbee) into main (5dbf1a3) will decrease coverage by 0.04%.
The diff coverage is 76.31%.

@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
- Coverage   86.50%   86.45%   -0.05%     
==========================================
  Files         133      133              
  Lines        6735     6755      +20     
==========================================
+ Hits         5826     5840      +14     
- Misses        909      915       +6     

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

is this a breaking change with respect to the return of evaluate?

Yes, I could return only a single number if the user passes only one metric but having different return types for different inputs is a bit strange

depends on #617

@guenthermi remember to tag @NicholasDunham for any future PRs that include docs (or docstrings)

two comments:

1. i don't like the fact that metrics='precision_at_k' should be supported, i suppose metrics should be always accept a list, not a str.
2. can we provide a all option to evaluate all metrics we have?

it's like what we're doing in callbacks, user should always pass a list of callbacks even user only need one. I'm expecting we:

.evaluate(metrics=['precision_at_k'])

I don't care much if we support passing only a string or not.
@JoanFM @samsja @JohannesMessner Do you have an opinion about it?

Having an all keyword is not a good idea, because the metric functions have different use cases, e.g., the ndcg metric should not be applied on a top k ranking while for other metrics this makes sense.

two comments:

1. i don't like the fact that metrics='precision_at_k' should be supported, i suppose metrics should be always accept a list, not a str.
2. can we provide a all option to evaluate all metrics we have?

it's like what we're doing in callbacks, user should always pass a list of callbacks even user only need one. I'm expecting we:

.evaluate(metrics=['precision_at_k'])

I don't care much if we support passing only a string or not. @JoanFM @samsja @JohannesMessner Do you have an opinion about it?

Having an all keyword is not a good idea, because the metric functions have different use cases, e.g., the ndcg metric should not be applied on a top k ranking while for other metrics this makes sense.

I believe, that to keep backwards compatibility we need to accept at least metric=STR but I agree, it is cleaner if user needs to pass list. But if it is complex, this seems like a minor issue to me

two comments:

1. i don't like the fact that metrics='precision_at_k' should be supported, i suppose metrics should be always accept a list, not a str.
2. can we provide a all option to evaluate all metrics we have?

it's like what we're doing in callbacks, user should always pass a list of callbacks even user only need one. I'm expecting we:

.evaluate(metrics=['precision_at_k'])

I don't care much if we support passing only a string or not. @JoanFM @samsja @JohannesMessner Do you have an opinion about it?
Having an all keyword is not a good idea, because the metric functions have different use cases, e.g., the ndcg metric should not be applied on a top k ranking while for other metrics this makes sense.

I believe, that to keep backwards compatibility we need to accept at least metric=STR but I agree, it is cleaner if user needs to pass list. But if it is complex, this seems like a minor issue to me

Agree, all is not good

make sense, ignore my all suggestion.

two comments:

1. i don't like the fact that metrics='precision_at_k' should be supported, i suppose metrics should be always accept a list, not a str.
2. can we provide a all option to evaluate all metrics we have?

it's like what we're doing in callbacks, user should always pass a list of callbacks even user only need one. I'm expecting we:

.evaluate(metrics=['precision_at_k'])

I don't care much if we support passing only a string or not. @JoanFM @samsja @JohannesMessner Do you have an opinion about it?
Having an all keyword is not a good idea, because the metric functions have different use cases, e.g., the ndcg metric should not be applied on a top k ranking while for other metrics this makes sense.

I believe, that to keep backwards compatibility we need to accept at least metric=STR but I agree, it is cleaner if user needs to pass list. But if it is complex, this seems like a minor issue to me

We could transform strings into a list of one string in the deprecation decorator and adjust the documentation. So I would not say that it is complex

two comments:

1. i don't like the fact that metrics='precision_at_k' should be supported, i suppose metrics should be always accept a list, not a str.
2. can we provide a all option to evaluate all metrics we have?

it's like what we're doing in callbacks, user should always pass a list of callbacks even user only need one. I'm expecting we:

.evaluate(metrics=['precision_at_k'])

I don't care much if we support passing only a string or not. @JoanFM @samsja @JohannesMessner Do you have an opinion about it?
Having an all keyword is not a good idea, because the metric functions have different use cases, e.g., the ndcg metric should not be applied on a top k ranking while for other metrics this makes sense.

I believe, that to keep backwards compatibility we need to accept at least metric=STR but I agree, it is cleaner if user needs to pass list. But if it is complex, this seems like a minor issue to me

We could transform strings into a list of one string in the deprecation decorator and adjust the documentation. So I would not say that it is complex

then go for it

I think that we can make the documentation a bit better for evaluation. @NicholasDunham what do u think ?

Maybe we can rewrite the documentation in another PR. There are also some old features of the evaluate functions which are not yet documented and today, I have not more time to do this.

Maybe we can rewrite the documentation in another PR. There are also some old features of the evaluate functions which are not yet documented and today, I have not more time to do this.

Okay then lets open an issue for improving this documentation and please align with @NicholasDunham on how do refactor it

Maybe we can rewrite the documentation in another PR. There are also some old features of the evaluate functions which are not yet documented and today, I have not more time to do this.

Okay then lets open an issue for improving this documentation and please align with @NicholasDunham on how do refactor it

I created #651 for the documentation changes.

📝 Docs are deployed on https://ft-feat-multiple-metrics-in-evaluate--jina-docs.netlify.app 🎉