Revert "Use window annotation for standard index" (#4117)

Reverts #4098 for breaking reader studies.

Author: pkcakeout

app/grandchallenge/core/utils/query.py

@@ -16,5 +16,6 @@ def index(queryset, obj):
 def set_seed(seed):
     # Note: this only works for postgres, if we ever switch dbs, this
     # may need changing.
-    with connection.cursor() as cursor:
-        cursor.execute(f"SELECT setseed({seed});")
+    cursor = connection.cursor()
+    cursor.execute(f"SELECT setseed({seed});")
+    cursor.close()

app/grandchallenge/reader_studies/models.py

@@ -15,7 +15,6 @@
 )
 from django.db import models
 from django.db.models import Avg, Count, Q, Sum
-from django.db.models.functions.window import RowNumber
 from django.db.models.signals import post_delete
 from django.dispatch import receiver
 from django.utils.functional import cached_property
@@ -54,7 +53,6 @@
     AccessRequestHandlingOptions,
     process_access_request,
 )
-from grandchallenge.core.utils.query import set_seed
 from grandchallenge.core.validators import JSONValidator
 from grandchallenge.core.vendored.django.validators import StepValueValidator
 from grandchallenge.hanging_protocols.models import (
@@ -892,56 +890,6 @@ def delete_reader_study_groups_hook(*_, instance: ReaderStudy, using, **__):
         pass
 
 
-class DisplaySetQuerySet(models.QuerySet):
-    def with_row_number(self):
-        return self.annotate(
-            row_number=models.Window(
-                expression=RowNumber(),
-                partition_by=models.F("reader_study"),
-                order_by=("order", "created"),
-            )
-        )
-
-    def with_shuffled_index(self, *, reader_study, seed):
-        """
-        Sets the shuffled index on each object
-
-        This requires fetching all pks for all of the display sets
-        in the reader study first
-        """
-        if not isinstance(seed, int) or seed < 1:
-            raise ValueError("Invalid seed")
-
-        queryset = self.filter(reader_study=reader_study)
-
-        set_seed(1 / seed)
-
-        queryset = queryset.annotate(
-            shuffled_index=models.Case(
-                *[
-                    models.When(pk=pk, then=models.Value(idx))
-                    for idx, pk in enumerate(
-                        queryset.order_by("?").values_list("pk", flat=True)
-                    )
-                ],
-                output_field=models.IntegerField(),
-            )
-        ).order_by("shuffled_index")
-
-        return queryset
-
-    def with_answer_count(self, user):
-        return self.annotate(
-            answer_count=Count(
-                "answers",
-                filter=Q(
-                    answers__is_ground_truth=False,
-                    answers__creator=user,
-                ),
-            )
-        )
-
-
 class DisplaySet(
     CIVSetStringRepresentationMixin,
     CIVSetObjectPermissionsMixin,
@@ -957,18 +905,6 @@ class DisplaySet(
     order = models.PositiveIntegerField(default=0)
     title = models.CharField(max_length=255, default="", blank=True)
 
-    objects = DisplaySetQuerySet.as_manager()
-
-    class Meta:
-        ordering = ("order", "created")
-        constraints = [
-            models.UniqueConstraint(
-                fields=["title", "reader_study"],
-                name="unique_display_set_title",
-                condition=~Q(title=""),
-            )
-        ]
-
     def assign_permissions(self):
         assign_perm(
             self.delete_perm,
@@ -991,6 +927,16 @@ def assign_permissions(self):
             self,
         )
 
+    class Meta:
+        ordering = ("order", "created")
+        constraints = [
+            models.UniqueConstraint(
+                fields=["title", "reader_study"],
+                name="unique_display_set_title",
+                condition=~Q(title=""),
+            )
+        ]
+
     @cached_property
     def is_editable(self):
         return not self.answers.exists()
@@ -1038,6 +984,10 @@ def description(self) -> str:
         else:
             return ""
 
+    @property
+    def standard_index(self) -> int:
+        return [*self.reader_study.display_sets.all()].index(self)
+
     @property
     def update_url(self):
         return reverse(

app/grandchallenge/reader_studies/serializers.py

@@ -115,19 +115,13 @@ class DisplaySetSerializer(HyperlinkedModelSerializer):
 
     def get_index(self, obj) -> int | None:
         if obj.reader_study.shuffle_hanging_list:
-            return obj.shuffled_index
-        else:
             try:
-                row_number = obj.row_number
-            except AttributeError:
-                # The annotation wasn't made when getting this object
-                row_number = (
-                    DisplaySet.objects.with_row_number()
-                    .get(pk=obj.pk)
-                    .row_number
-                )
-
-            return row_number - 1
+                return self.context["view"].randomized_qs.index(obj)
+            except ValueError:
+                # The list is empty if no reader study is specified.
+                return None
+        else:
+            return obj.standard_index
 
     class Meta:
         model = DisplaySet

app/grandchallenge/reader_studies/views.py

@@ -11,6 +11,7 @@
 from django.contrib.messages.views import SuccessMessageMixin
 from django.core.exceptions import PermissionDenied, ValidationError
 from django.db import transaction
+from django.db.models import Count, Q
 from django.forms import Form
 from django.forms.utils import ErrorList
 from django.http import (
@@ -73,6 +74,7 @@
 from grandchallenge.core.renderers import PaginatedCSVRenderer
 from grandchallenge.core.templatetags.random_encode import random_encode
 from grandchallenge.core.utils import strtobool
+from grandchallenge.core.utils.query import set_seed
 from grandchallenge.core.views import PermissionRequestUpdate
 from grandchallenge.datatables.views import Column
 from grandchallenge.groups.forms import EditorsForm
@@ -1018,11 +1020,11 @@ class DisplaySetViewSet(
     serializer_class = DisplaySetSerializer
     queryset = (
         DisplaySet.objects.all()
-        .with_row_number()
         .select_related("reader_study__hanging_protocol")
         .prefetch_related(
             "values__image",
             "values__interface",
+            "reader_study__display_sets",
             "reader_study__optional_hanging_protocols",
         )
     )
@@ -1033,6 +1035,7 @@ class DisplaySetViewSet(
         *api_settings.DEFAULT_RENDERER_CLASSES,
         PaginatedCSVRenderer,
     )
+    randomized_qs = []
 
     @property
     def reader_study(self):
@@ -1043,26 +1046,18 @@ def reader_study(self):
     def get_serializer_class(self):
         if self.action in ["partial_update", "update", "create"]:
             return DisplaySetPostSerializer
-        else:
-            return DisplaySetSerializer
-
-    def get_queryset(self):
-        queryset = super().get_queryset()
+        return DisplaySetSerializer
 
+    def list(self, request, *args, **kwargs):
+        queryset = self.filter_queryset(self.get_queryset())
         # Note: if more fields besides 'reader_study' are added to the
         # filter_set fields, we cannot call super anymore before randomizing
         # as we only want to filter out the display sets for a specific
         # reader study.
         reader_study = self.reader_study
-
-        if reader_study:
-            if reader_study.shuffle_hanging_list:
-                queryset = queryset.with_shuffled_index(
-                    reader_study=reader_study, seed=self.request.user.pk
-                )
-            else:
-                queryset = queryset.filter(reader_study=reader_study)
-
+        if reader_study and reader_study.shuffle_hanging_list:
+            queryset = queryset.filter(reader_study=reader_study)
+            queryset = self.create_randomized_qs(queryset=queryset)
         unanswered_by_user = strtobool(
             self.request.query_params.get("unanswered_by_user", "False")
         )
@@ -1073,10 +1068,8 @@ def get_queryset(self):
                 "Specifying a user is only possible when retrieving unanswered"
                 " display sets."
             )
-
         if username:
             user = get_user_model().objects.filter(username=username).get()
-
             if user != self.request.user and not self.request.user.has_perm(
                 "change_readerstudy", self.reader_study
             ):
@@ -1093,30 +1086,56 @@ def get_queryset(self):
                     "Please provide a reader study when filtering for "
                     "unanswered display_sets."
                 )
-
             answerable_question_count = reader_study.answerable_question_count
-            queryset = queryset.with_answer_count(user=user).exclude(
-                answer_count__gte=answerable_question_count,
+            queryset = (
+                queryset.annotate(
+                    answer_count=Count(
+                        "answers",
+                        filter=Q(
+                            answers__is_ground_truth=False,
+                            answers__creator=user,
+                        ),
+                    )
+                )
+                .exclude(
+                    answer_count__gte=answerable_question_count,
+                )
+                .order_by("order", "created")
             )
+            # Because the filtering has changed the list, we can no longer
+            # reapply .order_by("?"), as the ordering would not be consistent
+            # with the ordering of the full list. Instead, we use the
+            # previously saved randomized_qs and filter the proper items
+            # out of it.
+            if reader_study and reader_study.shuffle_hanging_list:
+                pks = queryset.values_list("pk", flat=True)
+                queryset = [x for x in self.randomized_qs if x.pk in pks]
 
-            if reader_study.shuffle_hanging_list:
-                queryset = queryset.order_by("shuffled_index")
-            else:
-                queryset = queryset.order_by("order", "created")
+        page = self.paginate_queryset(queryset)
+        if page is not None:
+            serializer = self.get_serializer(page, many=True)
+            return self.get_paginated_response(serializer.data)
 
-        return queryset
+        serializer = self.get_serializer(queryset, many=True)
+        return Response(serializer.data)
 
     def get_object(self):
         obj = super().get_object()
-
+        # retrieve the full queryset and save its shuffled version to later
+        # determine the shuffled index for this object
         if obj.reader_study.shuffle_hanging_list:
-            queryset = self.filter_queryset(self.get_queryset())
-            queryset = queryset.with_shuffled_index(
-                reader_study=obj.reader_study, seed=self.request.user.pk
-            )
-            return queryset.get(pk=obj.pk)
-        else:
-            return obj
+            queryset = self.get_queryset()
+            queryset = super().filter_queryset(queryset)
+            queryset = queryset.filter(reader_study=obj.reader_study)
+            self.create_randomized_qs(queryset=queryset)
+        return obj
+
+    def create_randomized_qs(self, queryset):
+        set_seed(1 / int(self.request.user.pk))
+        queryset = queryset.order_by("?")
+        # Save the queryset to determine each item's index in the serializer
+        self.randomized_qs = list(queryset)
+        return queryset
 
 
 class QuestionViewSet(ReadOnlyModelViewSet):

app/tests/reader_studies_tests/test_models.py

@@ -16,7 +16,6 @@
 from grandchallenge.reader_studies.models import (
     Answer,
     AnswerType,
-    DisplaySet,
     Question,
     QuestionWidgetKindChoices,
     ReaderStudy,
@@ -1277,51 +1276,3 @@ def test_reader_study_not_launchable_when_max_credits_consumed():
     assert reader_study.session_utilizations.first().credits_consumed == 500
     assert reader_study.credits_consumed == 500
     assert not reader_study.is_launchable
-
-
-@pytest.mark.django_db
-def test_with_row_number():
-    rs = ReaderStudyFactory()
-    ds1, ds2, ds3 = DisplaySetFactory.create_batch(3, reader_study=rs)
-
-    # Duplicate order should return unique indices
-    ds2.order = ds1.order
-    ds2.save()
-
-    ds4 = DisplaySetFactory()
-
-    # Test global access
-    queryset = DisplaySet.objects.with_row_number()
-    assert [*queryset] == [ds1, ds2, ds4, ds3]
-    assert {ds: ds.row_number for ds in queryset} == {
-        ds1: 1,
-        ds2: 2,
-        ds3: 3,
-        ds4: 1,
-    }
-
-    # Filtering should return the same indices
-    queryset = DisplaySet.objects.with_row_number().filter(reader_study=rs)
-    assert {ds: ds.row_number for ds in queryset} == {
-        ds1: 1,
-        ds2: 2,
-        ds3: 3,
-    }
-
-    # Getting a particular display set should return the correct order
-    last_display_set = (
-        DisplaySet.objects.with_row_number().filter(reader_study=rs).last()
-    )
-    assert last_display_set.row_number == 3
-
-    # Changing the order of the queryset should not change the index
-    queryset = DisplaySet.objects.with_row_number().order_by(
-        "-order", "-created"
-    )
-    assert [*queryset] == [ds3, ds4, ds2, ds1]
-    assert {ds: ds.row_number for ds in queryset} == {
-        ds1: 1,
-        ds2: 2,
-        ds3: 3,
-        ds4: 1,
-    }