From 6ce78e0c11a64939bcf0393e7849baa35028e7f9 Mon Sep 17 00:00:00 2001 From: Andrew Gardener Date: Fri, 2 Jun 2017 17:02:14 -0700 Subject: [PATCH] Allow all valid comparisons Previously comparisons would soft stop after a user had compared every answer at least once (allowing a user to perform about O(n) comparisons) Now The system will allow all valid combinations of comparisons to occur (allowing a user to perform n*(n-1)/2 or O(n^2) comparisons) Needed to allow instructors/teaching assistants to perform large amounts of comparisons --- .../pair/adaptive/pair_generator.py | 24 +------------ .../algorithms/pair/random/pair_generator.py | 24 +------------ compair/api/assignment.py | 10 +++--- compair/models/assignment.py | 15 ++++++++ compair/models/comparison.py | 34 ------------------- .../tests/algorithms/test_pair_adaptive.py | 13 ++++--- compair/tests/algorithms/test_pair_random.py | 13 ++++--- compair/tests/api/test_assignment.py | 8 ++--- compair/tests/api/test_comparisons.py | 16 +++++---- compair/tests/test_compair.py | 17 ++++++++-- 10 files changed, 67 insertions(+), 107 deletions(-) diff --git a/compair/algorithms/pair/adaptive/pair_generator.py b/compair/algorithms/pair/adaptive/pair_generator.py index 3a04b34b2..1d639a8b9 100644 --- a/compair/algorithms/pair/adaptive/pair_generator.py +++ b/compair/algorithms/pair/adaptive/pair_generator.py @@ -49,9 +49,6 @@ def generate_pair(self, scored_objects, comparison_pairs): if len(self.scored_objects) < 2: raise InsufficientObjectsForPairException - if self._has_compared_all(): - raise UserComparedAllObjectsException - comparison_pair = self._find_pair() if comparison_pair == None: @@ -59,30 +56,12 @@ def generate_pair(self, scored_objects, comparison_pairs): return comparison_pair - def _has_compared_all(self): - """ - Returns True if the user has already compared all objects, False otherwise. - """ - compared_keys = set() - for comparison_pair in self.comparison_pairs: - compared_keys.add(comparison_pair.key1) - compared_keys.add(comparison_pair.key2) - - all_keys = set([scored_object.key for scored_object in self.scored_objects]) - - # some comparison keys may have been soft deleted hence we need to use - # the set subtraction operation instead of comparing sizes - all_keys -= compared_keys - - return not all_keys - - def _find_pair(self): """ Returns an comparison pair by matching them up by score. - First key is selected by random within the lowest round possible - First key must have a valid opponent with the current user or else its skipped - - Second key canidates are filters by previous opponents to first key + - Second key candidates are filters by previous opponents to first key - Second key is selected by most similar score (randomly if tied) in the lowest round possible """ score_object_1 = None @@ -104,7 +83,6 @@ def _find_pair(self): if score_object_1 != None: break - # Note this should be caught in get_pair if score_object_1 == None: raise UserComparedAllObjectsException diff --git a/compair/algorithms/pair/random/pair_generator.py b/compair/algorithms/pair/random/pair_generator.py index a16c91aa5..9a1b5857b 100644 --- a/compair/algorithms/pair/random/pair_generator.py +++ b/compair/algorithms/pair/random/pair_generator.py @@ -44,9 +44,6 @@ def generate_pair(self, scored_objects, comparison_pairs): if len(self.scored_objects) < 2: raise InsufficientObjectsForPairException - if self._has_compared_all(): - raise UserComparedAllObjectsException - comparison_pair = self._find_pair() if comparison_pair == None: @@ -54,30 +51,12 @@ def generate_pair(self, scored_objects, comparison_pairs): return comparison_pair - def _has_compared_all(self): - """ - Returns True if the user has already compared all objects, False otherwise. - """ - compared_keys = set() - for comparison_pair in self.comparison_pairs: - compared_keys.add(comparison_pair.key1) - compared_keys.add(comparison_pair.key2) - - all_keys = set([scored_object.key for scored_object in self.scored_objects]) - - # some comparison keys may have been soft deleted hence we need to use - # the set subtraction operation instead of comparing sizes - all_keys -= compared_keys - - return not all_keys - - def _find_pair(self): """ Returns an comparison pair by matching them up randomly within a round. - First key is selected by random within the lowest round possible - First key must have a valid opponent with the current user or else its skipped - - Second key canidates are filters by previous opponents to first key + - Second key candidates are filters by previous opponents to first key - Second key is selected randomly in the lowest round possible """ score_object_1 = None @@ -99,7 +78,6 @@ def _find_pair(self): if score_object_1 != None: break - # Note this should be caught in get_pair if score_object_1 == None: raise UserComparedAllObjectsException diff --git a/compair/api/assignment.py b/compair/api/assignment.py index 866c50cbc..dc643a293 100644 --- a/compair/api/assignment.py +++ b/compair/api/assignment.py @@ -415,7 +415,8 @@ def get(self, course_uuid, assignment_uuid): .all() comparison_count = assignment.completed_comparison_count_for_user(current_user.id) - comparison_availble = Comparison.comparison_avialble_for_user(course.id, assignment.id, current_user.id) + other_student_answers = assignment.student_answer_count - answer_count + comparison_available = comparison_count < other_student_answers * (other_student_answers - 1) / 2 status = { 'answers': { @@ -425,7 +426,7 @@ def get(self, course_uuid, assignment_uuid): 'draft_ids': [draft.uuid for draft in drafts] }, 'comparisons': { - 'available': comparison_availble, + 'available': comparison_available, 'count': comparison_count, 'left': max(0, assignment.total_comparisons_required - comparison_count) } @@ -526,7 +527,8 @@ def get(self, course_uuid): ) assignment_drafts = [draft for draft in drafts if draft.assignment_id == assignment.id] comparison_count = assignment.completed_comparison_count_for_user(current_user.id) - comparison_availble = Comparison.comparison_avialble_for_user(course.id, assignment.id, current_user.id) + other_student_answers = assignment.student_answer_count - answer_count + comparison_available = comparison_count < other_student_answers * (other_student_answers - 1) / 2 statuses[assignment.uuid] = { 'answers': { @@ -536,7 +538,7 @@ def get(self, course_uuid): 'draft_ids': [draft.uuid for draft in assignment_drafts] }, 'comparisons': { - 'available': comparison_availble, + 'available': comparison_available, 'count': comparison_count, 'left': max(0, assignment.total_comparisons_required - comparison_count) } diff --git a/compair/models/assignment.py b/compair/models/assignment.py index 4e0cb2af3..cac860a44 100644 --- a/compair/models/assignment.py +++ b/compair/models/assignment.py @@ -193,6 +193,21 @@ def __declare_last__(cls): group="counts" ) + cls.student_answer_count = column_property( + select([func.count(Answer.id)]). + select_from(join(Answer, UserCourse, UserCourse.user_id == Answer.user_id)). + where(and_( + Answer.assignment_id == cls.id, + Answer.active == True, + Answer.draft == False, + Answer.practice == False, + UserCourse.course_id == cls.course_id, + UserCourse.course_role == CourseRole.student + )), + deferred=True, + group="counts" + ) + cls.top_answer_count = column_property( select([func.count(Answer.id)]). select_from(join(Answer, UserCourse, UserCourse.user_id == Answer.user_id)). diff --git a/compair/models/comparison.py b/compair/models/comparison.py index 5bf8c8a5b..38086d76b 100644 --- a/compair/models/comparison.py +++ b/compair/models/comparison.py @@ -76,40 +76,6 @@ def convert_to_comparison_pair(self): winning_key=self.winner_id ) - @classmethod - def comparison_avialble_for_user(cls, course_id, assignment_id, user_id): - from . import UserCourse, CourseRole, Answer - # ineligible authors - eg. instructors, TAs, dropped student, user - ineligible_users = UserCourse.query. \ - filter(and_( - UserCourse.course_id == course_id, - UserCourse.course_role != CourseRole.student - )). \ - values(UserCourse.user_id) - ineligible_user_ids_base = [u[0] for u in ineligible_users] - ineligible_user_ids_base.append(user_id) - - # ineligible authors (potentially) - eg. authors for answers that the user has seen - compared = Comparison.query \ - .filter_by( - user_id=user_id, - assignment_id=assignment_id - ) \ - .all() - compared_authors1 = [c.answer1.user_id for c in compared] - compared_authors2 = [c.answer2.user_id for c in compared] - ineligible_user_ids = ineligible_user_ids_base + compared_authors1 + compared_authors2 - - eligible_answers = Answer.query \ - .filter(and_( - Answer.assignment_id == assignment_id, - Answer.user_id.notin_(ineligible_user_ids), - Answer.draft == False, - Answer.active == True - )) \ - .count() - return eligible_answers / 2 >= 1 # min 1 pair required - @classmethod def __declare_last__(cls): super(cls, cls).__declare_last__() diff --git a/compair/tests/algorithms/test_pair_adaptive.py b/compair/tests/algorithms/test_pair_adaptive.py index 281b8853f..37991701f 100644 --- a/compair/tests/algorithms/test_pair_adaptive.py +++ b/compair/tests/algorithms/test_pair_adaptive.py @@ -69,12 +69,15 @@ def test_generate_pair(self, mock_shuffle): rounds=3, wins=None, loses=None, opponents=None ) ] - comparisons = [ - ComparisonPair(1,2,None), - ComparisonPair(1,3,None), - ComparisonPair(1,4,None) - ] + comparisons = [] + max_comparisons = 6 # n(n-1)/2 = 4*3/2 + for i in range(0, max_comparisons): + results = self.pair_algorithm.generate_pair(scored_objects, comparisons) + comparisons.append( + ComparisonPair(results.key1, results.key2, results.key1) + ) + # if trying to run one more time, should run into all objects compared error with self.assertRaises(UserComparedAllObjectsException): results = self.pair_algorithm.generate_pair(scored_objects, comparisons) diff --git a/compair/tests/algorithms/test_pair_random.py b/compair/tests/algorithms/test_pair_random.py index 797e48054..6ddfc26d2 100644 --- a/compair/tests/algorithms/test_pair_random.py +++ b/compair/tests/algorithms/test_pair_random.py @@ -69,12 +69,15 @@ def test_generate_pair(self, mock_shuffle): rounds=3, wins=None, loses=None, opponents=None ) ] - comparisons = [ - ComparisonPair(1,2,None), - ComparisonPair(1,3,None), - ComparisonPair(1,4,None) - ] + comparisons = [] + max_comparisons = 6 # n(n-1)/2 = 4*3/2 + for i in range(0, max_comparisons): + results = self.pair_algorithm.generate_pair(scored_objects, comparisons) + comparisons.append( + ComparisonPair(results.key1, results.key2, results.key1) + ) + # if trying to run one more time, should run into all objects compared error with self.assertRaises(UserComparedAllObjectsException): results = self.pair_algorithm.generate_pair(scored_objects, comparisons) diff --git a/compair/tests/api/test_assignment.py b/compair/tests/api/test_assignment.py index da855ec4f..05f05e56d 100644 --- a/compair/tests/api/test_assignment.py +++ b/compair/tests/api/test_assignment.py @@ -370,8 +370,8 @@ def _submit_all_possible_comparisons_for_user(self, user_id): for answer in self.data.get_student_answers(): if answer.assignment_id == self.assignment.id and answer.user_id != user_id: num_eligible_answers += 1 - # n - 1 possible pairs before all answers have been compared - num_possible_comparisons = num_eligible_answers - 1 + # n(n-1)/2 possible pairs before all answers have been compared + num_possible_comparisons = int(num_eligible_answers * (num_eligible_answers - 1) / 2) for i in range(num_possible_comparisons): comparisons = Comparison.create_new_comparison_set(self.assignment.id, user_id, False) for comparison in comparisons: @@ -462,8 +462,8 @@ def _submit_all_possible_comparisons_for_user(self, user_id): for answer in self.data.get_student_answers(): if answer.assignment_id == self.assignment.id and answer.user_id != user_id: num_eligible_answers += 1 - # n - 1 possible pairs before all answers have been compared - num_possible_comparisons = num_eligible_answers - 1 + # n(n-1)/2 possible pairs before all answers have been compared + num_possible_comparisons = int(num_eligible_answers * (num_eligible_answers - 1) / 2) for i in range(num_possible_comparisons): comparisons = Comparison.create_new_comparison_set(self.assignment.id, user_id, False) for comparison in comparisons: diff --git a/compair/tests/api/test_comparisons.py b/compair/tests/api/test_comparisons.py index 0582dcd1d..1dc93ec35 100644 --- a/compair/tests/api/test_comparisons.py +++ b/compair/tests/api/test_comparisons.py @@ -229,15 +229,19 @@ def test_get_and_submit_comparison(self, mocked_update_assignment_grades_run, mo users = [self.data.get_authorized_student(), self.data.get_authorized_instructor(), self.data.get_authorized_ta()] for user in users: - compared_answer_uuids = set() + max_comparisons = 0 + other_student_answers = 0 valid_answer_uuids = set() for answer in self.data.get_student_answers(): if answer.assignment.id == self.assignment.id and answer.user_id != user.id: + other_student_answers += 1 valid_answer_uuids.add(answer.uuid) + max_comparisons = int(other_student_answers * (other_student_answers - 1) / 2) if user.id == self.data.get_authorized_student().id: for comparison_example in self.data.comparisons_examples: if comparison_example.assignment_id == self.assignment.id: + max_comparisons += 1 valid_answer_uuids.add(comparison_example.answer1_uuid) valid_answer_uuids.add(comparison_example.answer2_uuid) @@ -254,7 +258,7 @@ def test_get_and_submit_comparison(self, mocked_update_assignment_grades_run, mo db.session.commit() current = 0 - while len(valid_answer_uuids - compared_answer_uuids) > 0: + while current < max_comparisons: current += 1 if user.id == self.data.get_authorized_student().id: course_grade = CourseGrade.get_user_course_grade(self.course, user).grade @@ -308,8 +312,6 @@ def test_get_and_submit_comparison(self, mocked_update_assignment_grades_run, mo content_type='application/json') self.assert200(rv) actual_comparisons = rv.json['objects'] - compared_answer_uuids.add(actual_comparisons[0]['answer1_id']) - compared_answer_uuids.add(actual_comparisons[0]['answer2_id']) self._validate_comparison_submit(comparison_submit, actual_comparisons, expected_comparisons) # grades should increase for every comparison @@ -401,8 +403,8 @@ def _submit_all_possible_comparisons_for_user(self, user_id): for answer in self.data.get_student_answers(): if answer.assignment_id == self.assignment.id and answer.user_id != user_id: num_eligible_answers += 1 - # n - 1 possible pairs before all answers have been compared - num_possible_comparisons = num_eligible_answers - 1 + # n(n-1)/2 possible pairs before all answers have been compared + num_possible_comparisons = int(num_eligible_answers * (num_eligible_answers - 1) / 2) winner_ids = [] loser_ids = [] for i in range(num_possible_comparisons): @@ -461,7 +463,7 @@ def test_score_calculation(self, mock_shuffle): # Check that ranking by score and by wins match, this only works for low number of # comparisons - sorted_expect_ranking = sorted(num_wins_by_id.items(), key=operator.itemgetter(0), reverse=True) + sorted_expect_ranking = sorted(num_wins_by_id.items(), key=operator.itemgetter(0)) sorted_expect_ranking = sorted(sorted_expect_ranking, key=operator.itemgetter(1)) expected_ranking_by_wins = [answer_id for (answer_id, wins) in sorted_expect_ranking] diff --git a/compair/tests/test_compair.py b/compair/tests/test_compair.py index 825daff13..d8e4a1371 100644 --- a/compair/tests/test_compair.py +++ b/compair/tests/test_compair.py @@ -4,6 +4,8 @@ import unittest import mock import uuid +import sys +import os from flask_testing import TestCase from os.path import dirname @@ -64,6 +66,15 @@ def collect_from_response(resp_key, response): return wrapper return _decorator +@contextmanager +def suppress_stdout(): + old_stdout = sys.stdout + sys.stdout = open(os.devnull, "w") + try: + yield + finally: + sys.stdout.close() + sys.stdout = old_stdout class ComPAIRTestCase(TestCase): def create_app(self): @@ -72,7 +83,8 @@ def create_app(self): def setUp(self): db.create_all() - populate(default_data=True) + with suppress_stdout(): + populate(default_data=True) # reset login settings in case tests fail self.app.config['APP_LOGIN_ENABLED'] = True self.app.config['CAS_LOGIN_ENABLED'] = True @@ -197,7 +209,8 @@ def get_url(self, **values): class ComPAIRAPIDemoTestCase(ComPAIRAPITestCase): def setUp(self): db.create_all() - populate(default_data=True, sample_data=True) + with suppress_stdout(): + populate(default_data=True, sample_data=True) self.app.config['DEMO_INSTALLATION'] = True