Add MRR, MAP, DCG, nDCG #46

abheesht17 · 2025-04-09T15:43:51Z

TODO:

Add UTs.
Add documentation.
Verify doc-strings.
Some small TODOs in the code, which can be decided after code review.
Add get_config() for DCG and nDCG

abheesht17 · 2025-04-11T17:19:43Z

MRR ready for review

hertschuh

Thanks!

Partial review... to be continued

hertschuh · 2025-04-11T22:03:30Z

keras_rs/src/metrics/mean_reciprocal_rank.py

+from keras_rs.src.utils.ranking_metrics_utils import sort_by_scores
+
+
+@keras_rs_export("keras_rs.losses.MeanReciprocalRank")


Should be keras_rs.metrics.MeanReciprocalRank.

hertschuh · 2025-04-11T22:05:52Z

keras_rs/src/utils/ranking_metrics_utils.py

+    Returns:
+        List of sorted tensors (`tensors_to_sort`), sorted using `scores`.
+    """
+    # TODO: Consider exposing `shuffle_ties` to the user.


It looks like it's done, remove TODO for shuffle_ties

hertschuh · 2025-04-14T02:10:17Z

keras_rs/src/metrics/ranking_metric.py

+                shape `(list_size)` or `(batch_size, list_size)`. Defaults to
+                `None`.
+        """
+        # TODO (abheesht): Should `y_true` be a dict, with `"mask"` as one key


You mean as an option? Right now, there's no way to pass a mask, right?

hertschuh · 2025-04-14T02:11:20Z

keras_rs/src/metrics/ranking_metric.py

+        if isinstance(y_pred, list):
+            y_pred = ops.convert_to_tensor(y_pred)
+        # `sample_weight` can be a scalar too.
+        if isinstance(sample_weight, (list, float, int)):


Remove all 3 if isinstance(...) and just call ops.convert_to_tensor, it does the if for you.

hertschuh · 2025-04-14T02:13:35Z

keras_rs/src/metrics/ranking_metric.py

+        elif sample_weight_rank == 2:
+            check_shapes_compatible(sample_weight_shape, y_true_shape)
+
+        # Want to make sure `sample_weight` is of the same shape as


Meaning you should add a check here?

hertschuh · 2025-04-14T02:20:37Z

keras_rs/src/utils/keras_utils.py

+def check_rank(
+    x_rank: int,
+    allowed_ranks: tuple[int, ...] = (1, 2),
+    tensor_name: Optional[str] = None,


It look like tensor_name is not really optional.

hertschuh · 2025-04-14T02:21:37Z

keras_rs/src/utils/keras_utils.py

+# Check ranks and shapes.
+def check_rank(
+    x_rank: int,
+    allowed_ranks: tuple[int, ...] = (1, 2),


I wouldn't provide a default for allowed_ranks. When I'm reading the code, I don't want to have to go to the definition to know what ranks I'm checking, I should have them right there where it's called.

hertschuh · 2025-04-14T02:24:32Z

keras_rs/src/utils/loss_and_metric_utils.py

+from keras_rs.src.utils.keras_utils import check_shapes_compatible
+
+
+def process_inputs(


This have a more specific name, maybe something like standardize_ranks or check_ranks_and_shapes.

hertschuh · 2025-04-14T02:25:16Z

keras_rs/src/utils/loss_and_metric_utils.py

+    """
+    Utility function for processing inputs for losses and metrics.
+
+    This utility function does three things:


Add Args and Return section.

hertschuh · 2025-04-14T02:29:27Z

keras_rs/src/utils/ranking_metrics_utils.py

+    if k is None:
+        k = max_possible_k
+    else:
+        k = ops.minimum(k, max_possible_k)


I would split this in 2, because JAX will want a static int value and TensorFlow may need a dynamic value when max_possible_k is a scalar tensor:

if k is None: k = max_possible_k elif isinstance(max_possible_k, int): k = min(k, max_possible_k) else: k = ops.minimum(k, max_possible_k)

hertschuh

Thanks for this big PR. A lot of work obviously went into this!

More comments...

hertschuh · 2025-04-14T18:06:56Z

keras_rs/src/utils/ranking_metrics_utils.py

+      everywhere, even for lists without any relevant examples because
+      `sum(per_list_weights) ==  num(sum(relevance) != 0)`. This handles the
+      standard ranking metrics where the weights are all
+      1.0.


nitpick, move to previous line.

hertschuh · 2025-04-14T18:07:48Z

keras_rs/src/utils/ranking_metrics_utils.py

+      num(sum(relevance) != 0) / num(lists)
+      ```
+
+      The rest have weights 1.0 / num(lists).


nit: put formula between single quotes.

hertschuh · 2025-04-14T18:09:24Z

keras_rs/src/utils/ranking_metrics_utils.py

+    nonzero_relevance = ops.where(
+        nonzero_weights,
+        ops.cast(nonzero_relevance_condition, "float32"),
+        ops.zeros_like(per_list_relevance),


Is there a reason to do zeros_like rather than just 0.0? It should give the same result. But is it for ragged tensors or something?

hertschuh · 2025-04-14T18:10:57Z

keras_rs/src/utils/ranking_metrics_utils.py

+
+    # Identify lists where both weights and relevance sums are non-zero.
+    nonzero_relevance_condition = ops.greater(per_list_relevance, 0.0)
+    nonzero_relevance = ops.where(


It looks like this is just a ops.logical_and between nonzero_relevance and nonzero_weights.

A logical_and or even a multiply should be faster than a where.

hertschuh · 2025-04-14T18:12:18Z

keras_rs/src/utils/ranking_metrics_utils.py

+
+    # Calculate the per-list weights using the core formula
+    # Numerator: sum(weights * relevance) per list
+    numerator = ops.sum(weights * relevance, axis=1, keepdims=True)


ops.multiply(weights, relevance)

hertschuh · 2025-04-14T18:25:37Z

keras_rs/src/utils/ranking_metrics_utils.py

+
+
+def default_rank_discount_fn(rank: types.Tensor) -> types.Tensor:
+    return ops.divide(ops.log(2.0), ops.log1p(rank))


The docstring above says it's equivalent to lambda rank: log2(rank + 1).

So that would be reversing the arguments of the divide. Also, it would be clearer if written as ops.log2(rank + 1.0).

hertschuh · 2025-04-14T18:29:28Z

keras_rs/src/metrics/n_dcg.py

+
+
+@keras_rs_export("keras_rs.metrics.nDCG")
+class nDCG(RankingMetric):


The convention is to always use an upper case letter for the first letter of a class, so it should be NDCG, even if it's written as nDCG in papers.

Also the file should be ndcg.py.

I don't actually understand why it's written nDCG since all the letters form an acronym anyway.

hertschuh · 2025-04-14T18:33:25Z

keras_rs/src/utils/ranking_metrics_utils.py

@@ -0,0 +1,240 @@
+from typing import Callable, Optional


I'm not a huge fan of the concept of a utils folder in general, and I typically prefer the utils files to be in the same folder as where it's used. So for instance, I think this could be in keras_rs/src/metrics.

What do you think? Note that keras itself is not consistent on this.

hertschuh · 2025-04-14T18:33:55Z

keras_rs/src/utils/pairwise_loss_utils.py

@@ -1,9 +1,8 @@
-from typing import Callable, Optional
+from typing import Callable


I'm not a huge fan of the concept of a utils folder in general, and I typically prefer the utils files to be in the same folder as where it's used. So for instance, I think this could be in keras_rs/src/losses.

What do you think? Note that keras itself is not consistent on this.

hertschuh · 2025-04-14T18:34:57Z

keras_rs/src/utils/loss_and_metric_utils.py

@@ -0,0 +1,62 @@
+from typing import Optional


I'm not a huge fan of the concept of a utils folder in general, and I typically prefer the utils files to be in the same folder as where it's used. This one is a bit funny because it's for both metrics and losses, but I think it wouldn't be too shocking to put it in keras_rs/src/metrics since a loss is just a kind of metric.

What do you think? Note that keras itself is not consistent on this.

abheesht17 added 6 commits April 9, 2025 15:17

Add initial version of MRR

6c88424

Fix max op

2ea8f3b

Fix min op

38ac42f

Minor fixes

7105cee

Fixes

3462b47

Remove print statements

8f33613

abheesht17 closed this Apr 9, 2025

abheesht17 reopened this Apr 9, 2025

abheesht17 changed the title ~~Add MRR~~ Add MRR, MAP, DCG, nDCG Apr 10, 2025

abheesht17 added 2 commits April 10, 2025 13:42

Add MAP, DCG, nDCG

96e4469

Add documentation and unit tests for MRR

e296c48

abheesht17 marked this pull request as ready for review April 11, 2025 06:56

abheesht17 requested a review from hertschuh April 11, 2025 06:56

abheesht17 added 2 commits April 11, 2025 12:33

Fix UT

5bcfab8

Fix JAX, Torch UT

9d1d328

abheesht17 added 10 commits April 13, 2025 08:36

Add MAP UTs

e8b021a

Better UTs for MAP

2f72354

Add UTs for DCG

ce282c5

Add nDCG UTs

ffa4b47

Make UTs simpler

82fb0d0

Add evaluate() UT

5ddabda

Remove print statements

2257b23

Add doc-strings for DCG, nDCG

fedf1b6

Small doc-string edit

9acfc08

Change abstract method error raising

3e60d92

hertschuh reviewed Apr 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MRR, MAP, DCG, nDCG #46

Add MRR, MAP, DCG, nDCG #46

abheesht17 commented Apr 9, 2025 •

edited

Loading

abheesht17 commented Apr 11, 2025

hertschuh left a comment

hertschuh Apr 11, 2025

hertschuh Apr 11, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh left a comment

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

hertschuh Apr 14, 2025

		from keras_rs.src.utils.ranking_metrics_utils import sort_by_scores


		@keras_rs_export("keras_rs.losses.MeanReciprocalRank")

		from keras_rs.src.utils.keras_utils import check_shapes_compatible


		def process_inputs(



		def default_rank_discount_fn(rank: types.Tensor) -> types.Tensor:
		return ops.divide(ops.log(2.0), ops.log1p(rank))



		@keras_rs_export("keras_rs.metrics.nDCG")
		class nDCG(RankingMetric):

		@@ -1,9 +1,8 @@
		from typing import Callable, Optional
		from typing import Callable

Add MRR, MAP, DCG, nDCG #46

Are you sure you want to change the base?

Add MRR, MAP, DCG, nDCG #46

Conversation

abheesht17 commented Apr 9, 2025 • edited Loading

abheesht17 commented Apr 11, 2025

hertschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hertschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abheesht17 commented Apr 9, 2025 •

edited

Loading