Python evaluator module fix

[loomlike]

Description

Python evaluation module' ranking metric functions have redundant and unnecessary sorting codes.
E.g.

df_hit["rank"] = df_hit.groupby(col_user)[col_prediction].rank(
        method="first", ascending=False
)

doesn't need to use rank() since df_hit is already sorted by user and ratings as it is generated by groupby user (pandas groupby's sort argument is by default True) and nlargest ratings.

This change removes those redundant and unnecessary sorts and also refactor get_top_k_items to return DataFrame with 'rank' column to make its behavior the same as our pyspark evaluation module.

Related Issues

Checklist:

• I have followed the contribution guidelines and code style for this project.
• I have added tests covering my contributions.
• I have updated the documentation accordingly.

[miguelgfierro]

LGTM

[gramhagen]

this is great, small improvement suggested

[gramhagen]

you can avoid the repeated groupby too

groups = dataframe.groupby(col_user, as_index=False)
top_k_items = groups.apply(lambda x: x.nlargest(k, col_rating)).reset_index(drop=True)
top_k_items["rank"] = groups.cumcount() + 1

[gramhagen]

i would remove the double quotes from rank to match just the backticks like col_user

[gramhagen]

also, in the returns section of get_top_k_items =)

[loomlike]

good catch!

[yueguoguo]

Great

[gramhagen]

one more "rank" is there, if you can fix that then we're good

[loomlike]

@gramhagen Few changes since the last review:

1. changed "rank" to rank
2. caching groups turns out does not work, since nlargest sorts the ratings while the cached group object still contains unsorted ratings. I changed it back to use groupby again, but added sort=False so that groupby can be performed efficiently (groupby-without-sorting still keeps the inter-group orders and we already sorted previously by 'nlargest')
3. found the above issue from spark's unit-tests which matches spark-evaluation-fn results to python's. Python evaluation tests couldn't catch the error because the test case users and items were already sorted. I made a simple tweak to the test case so that can catch such errors in the future.

[gramhagen]

oh interesting, i didn't realize we use the python evaluation to validate test results for spark, we should remove that linkage, I'll add a separate feature request

[gramhagen]

oh, i take it back, I guess that's an additional check just to ensure they match. i guess it helped in this case.

[miguelgfierro]

@loomlike feel free to merge when you think it is convenient