From ab40748b5aa24ece6fb1e7eb8bfce24fdbbce286 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 24 Oct 2019 15:28:09 -0400 Subject: [PATCH 1/3] Add an implementation of python2 style round. Python 3 has no way to access this implementation of rounding in the standard library. We implement it here so that we can continue to use it for grades calculation to keep regrading consistent. Currently we can't be confident that if we change the rounding behaviour we won't impact students. We can't be sure that students that were previously passing wouldn't suddenly no longer be passing. Given this, it's lower risk to just implement the old rounding strategy here and use it when we are rounding to calculate grades. --- openedx/core/lib/grade_utils.py | 24 ++++++++++++++++++++++ openedx/core/lib/tests/test_grade_utils.py | 14 ++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/openedx/core/lib/grade_utils.py b/openedx/core/lib/grade_utils.py index 601b994d13..2e7073e962 100644 --- a/openedx/core/lib/grade_utils.py +++ b/openedx/core/lib/grade_utils.py @@ -1,6 +1,7 @@ """ Helpers functions for grades and scores. """ +import math def compare_scores(earned1, possible1, earned2, possible2, treat_undefined_as_zero=False): @@ -42,3 +43,26 @@ def is_score_higher_or_equal(earned1, possible1, earned2, possible2, treat_undef """ is_higher_or_equal, _, _ = compare_scores(earned1, possible1, earned2, possible2, treat_undefined_as_zero) return is_higher_or_equal + + +def round_away_from_zero(number, digits=0): + """ + Round numbers using the 'away from zero' strategy as opposed to the + 'Banker's rounding strategy.' The strategy refers to how we round when + a number is half way between two numbers. eg. 0.5, 1.5, etc. In python 2 + positive numbers in this category would be rounded up and negative numbers + would be rounded down. ie. away from zero. In python 3 numbers round + towards even. So 0.5 would round to 0 but 1.5 would round to 2. + + See here for more on floating point rounding strategies: + https://en.wikipedia.org/wiki/IEEE_754#Rounding_rules + + We want to continue to round away from zero so that student grades remain + consistent and don't suddenly change. + """ + p = 10.0 ** digits + + if number >= 0: + return float(math.floor((number * p) + 0.5)) / p + else: + return float(math.ceil((number * p) - 0.5)) / p diff --git a/openedx/core/lib/tests/test_grade_utils.py b/openedx/core/lib/tests/test_grade_utils.py index 9c4f08216d..4d6dd00912 100644 --- a/openedx/core/lib/tests/test_grade_utils.py +++ b/openedx/core/lib/tests/test_grade_utils.py @@ -7,7 +7,7 @@ from unittest import TestCase import ddt -from ..grade_utils import compare_scores +from ..grade_utils import compare_scores, round_away_from_zero @ddt.ddt @@ -45,3 +45,15 @@ class TestGradeUtils(TestCase): assert is_higher is True assert 0 == percentage_1 assert 0 == percentage_2 + + @ddt.data( + (0.5, 1), + (1.45, 1.5, 1), + (-0.5, -1.0), + (-0.1, -0.0), + (0.1, 0.0), + (0.0, 0.0) + ) + @ddt.unpack + def test_round_away_from_zero(self, precise, expected_rounded_number, rounding_precision=0): + assert round_away_from_zero(precise, rounding_precision) == expected_rounded_number From 9f9d90459204526b8609e985929b7898f57b3661 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 24 Oct 2019 15:47:10 -0400 Subject: [PATCH 2/3] Update code to use the new rounding method. See the commit that added round_away_from_zero for details. IN the case of dashboard_data.py: We don't first cast to decimal here because in python 2 this didn't do anything. The python 2 rounding method would just cast things to float before doing the rounding anyway so it just wastes cpu cycles. In python 3 this causes some typing issues so just removing it. --- common/lib/capa/capa/responsetypes.py | 7 ++++--- common/lib/xmodule/xmodule/lti_2_util.py | 3 ++- lms/djangoapps/class_dashboard/dashboard_data.py | 9 +++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index cd2b81fc1c..86f99f2674 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -48,6 +48,7 @@ import capa.safe_exec as safe_exec import capa.xqueue_interface as xqueue_interface from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib import edx_six +from openedx.core.lib.grade_utils import round_away_from_zero from . import correctmap from .registry import TagRegistry @@ -727,7 +728,7 @@ class ChoiceResponse(LoncapaResponse): good_non_answers = sum([1 for blank in student_non_answers if blank in self.incorrect_choices]) edc_current_grade = good_answers + good_non_answers - return_grade = round(self.get_max_score() * float(edc_current_grade) / float(edc_max_grade), 2) + return_grade = round_away_from_zero(self.get_max_score() * float(edc_current_grade) / float(edc_max_grade), 2) if edc_current_grade == edc_max_grade: return CorrectMap(self.answer_id, correctness='correct') @@ -764,10 +765,10 @@ class ChoiceResponse(LoncapaResponse): return_grade = self.get_max_score() return CorrectMap(self.answer_id, correctness='correct', npoints=return_grade) elif halves_error_count == 1 and len(all_choices) > 2: - return_grade = round(self.get_max_score() / 2.0, 2) + return_grade = round_away_from_zero(self.get_max_score() / 2.0, 2) return CorrectMap(self.answer_id, correctness='partially-correct', npoints=return_grade) elif halves_error_count == 2 and len(all_choices) > 4: - return_grade = round(self.get_max_score() / 4.0, 2) + return_grade = round_away_from_zero(self.get_max_score() / 4.0, 2) return CorrectMap(self.answer_id, correctness='partially-correct', npoints=return_grade) else: return CorrectMap(self.answer_id, 'incorrect') diff --git a/common/lib/xmodule/xmodule/lti_2_util.py b/common/lib/xmodule/xmodule/lti_2_util.py index 0e15fd1f8b..d32f7a41f9 100644 --- a/common/lib/xmodule/xmodule/lti_2_util.py +++ b/common/lib/xmodule/xmodule/lti_2_util.py @@ -18,6 +18,7 @@ from oauthlib.oauth1 import Client from six import text_type from webob import Response from xblock.core import XBlock +from openedx.core.lib.grade_utils import round_away_from_zero log = logging.getLogger(__name__) @@ -177,7 +178,7 @@ class LTI20ModuleMixin(object): return Response(json.dumps(base_json_obj).encode('utf-8'), content_type=LTI_2_0_JSON_CONTENT_TYPE) # Fall through to returning grade and comment - base_json_obj['resultScore'] = round(self.module_score, 2) + base_json_obj['resultScore'] = round_away_from_zero(self.module_score, 2) base_json_obj['comment'] = self.score_comment return Response(json.dumps(base_json_obj).encode('utf-8'), content_type=LTI_2_0_JSON_CONTENT_TYPE) diff --git a/lms/djangoapps/class_dashboard/dashboard_data.py b/lms/djangoapps/class_dashboard/dashboard_data.py index a53007b2cc..038625b266 100644 --- a/lms/djangoapps/class_dashboard/dashboard_data.py +++ b/lms/djangoapps/class_dashboard/dashboard_data.py @@ -16,6 +16,7 @@ from lms.djangoapps.instructor_analytics.csvs import create_csv_response from util.json_request import JsonResponse from xmodule.modulestore.django import modulestore from xmodule.modulestore.inheritance import own_metadata +from openedx.core.lib.grade_utils import round_away_from_zero # Used to limit the length of list displayed to the screen. MAX_SCREEN_LIST_LENGTH = 250 @@ -193,7 +194,7 @@ def get_d3_problem_grade_distrib(course_id): for (grade, count_grade) in problem_info['grade_distrib']: percent = 0.0 if max_grade > 0: - percent = round((grade * 100.0) / max_grade, 1) + percent = round_away_from_zero((grade * 100.0) / max_grade, 1) # Compute percent of students with this grade student_count_percent = 0 @@ -352,7 +353,7 @@ def get_d3_section_grade_distrib(course_id, section): for (grade, count_grade) in grade_distrib[problem]['grade_distrib']: percent = 0.0 if max_grade > 0: - percent = round((grade * 100.0) / max_grade, 1) + percent = round_away_from_zero((grade * 100.0) / max_grade, 1) # Construct tooltip for problem in grade distibution view tooltip = { @@ -513,7 +514,7 @@ def get_students_problem_grades(request, csv=False): student_dict['percent'] = 0 if student['max_grade'] > 0: - student_dict['percent'] = round(student['grade'] * 100 / student['max_grade']) + student_dict['percent'] = round_away_from_zero(student['grade'] * 100 / student['max_grade']) results.append(student_dict) max_exceeded = False @@ -535,7 +536,7 @@ def get_students_problem_grades(request, csv=False): for student in students: percent = 0 if student['max_grade'] > 0: - percent = round(decimal.Decimal(student['grade'] * 100 / student['max_grade']), 1) + percent = round_away_from_zero((student['grade'] * 100 / student['max_grade']), 1) results.append([student['student__profile__name'], student['student__username'], student['grade'], percent]) response = create_csv_response(filename, header, results) From facd737ee47a5f58505b602c4be6bf637ff25426 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 24 Oct 2019 15:00:35 -0400 Subject: [PATCH 3/3] Update code to use the new rounding method in course_grades. So making this code change took a few hours. But then deciding that it was the right one of the many options available took the next 3 days. When changing to the new rounding function, we had a test that started failing. It appears as if our new rounding function is not the same in some way as the one built into python 2. ``` >>> round(.0045*100 + .05)/100 0.0 >>> round_away_from_zero(.0045*100 + .05)/100 0.01 ``` Doing the math by hand we see that the new function is actually correct but the old one is clearly rounding incorrectly in this case. Looking closer at this I discovered that it was due to a floating point issue where .0045*100 is represented as 0.44999999999999996 so when we add 0.05 to this number we get 0.49999999999999994. This is all because of the limitations of floating point arithmetic. See https://docs.python.org/3/tutorial/floatingpoint.html#tut-fp-issues for more on that. Because python does its rounding at the bit level in C code. It treats the .4999... as below the .5 cutoff and rounds down. Whereas our code does more simple arithmetic which causes the number to correct itself before we round and so correctly rounds up to 0.01 The result of this change is that previously, the rounding threshold used to be that any number > .0045 would ronud to 0.01 and now any number that is >= .0045 rounds to 0.01 Note that if we only care about the two most significant digits of number between 0 and 1, this error only manifests itself in such a way that other than the case of going from 0.00 to 0.01 eg. from 0% to 1% none of the other cases where we would now round up cause the 2 most significant digits to change. Given this level of impact, we're fine with this change. In our tests we see this for one case, where an incomplete turns into an F in a test. I'm updating the test here to be more correct. As we were looking at it we speculated as to why we were adding the .05 to the number. Could it be to counteract this floating point issue? It turns out not. Looking at this commit(a1286b1c7db979c9ab4bd89c7feef2) we see that it looks like this was intended to always round up to the nearest percentage point. However, there's a typo here. If you wanted to ensure that we always rounded up to the nearest percentage point you would have the math be `round(final_grade_percent*100 + 0.5)/ 100` or a simpler way to do this would be `math.ceil(final_grade_percent*100)/100`. However, that is not what happened and 7 years later, there have been a lot of people graded with the wrong rounding where essentialy anyone with a grade of 89.45 gets a 90 when the intended impact was supposed to be that anyone with a grade above an 89.0 would get a grade of 90. Changing it now requires a lot of conversation and wolud have a large impact on existing learners. So we are not going to change it as a part of the python 2 -> python 3 upgrade. I have created https://openedx.atlassian.net/browse/TNL-6972 to capture this issue if we want to address it in the future. --- lms/djangoapps/grades/course_grade.py | 5 ++++- lms/djangoapps/instructor/tests/test_spoc_gradebook.py | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/grades/course_grade.py b/lms/djangoapps/grades/course_grade.py index dcc5bf363b..fb8181d7da 100644 --- a/lms/djangoapps/grades/course_grade.py +++ b/lms/djangoapps/grades/course_grade.py @@ -12,6 +12,7 @@ from django.conf import settings from django.utils.encoding import python_2_unicode_compatible from lazy import lazy +from openedx.core.lib.grade_utils import round_away_from_zero from xmodule import block_metadata_utils from .config import assume_zero_if_absent @@ -296,7 +297,9 @@ class CourseGrade(CourseGradeBase): Computes and returns the grade percentage from the given result from the grader. """ - return round(grader_result['percent'] * 100 + 0.05) / 100 + + # Confused about the addition of .05 here? See https://openedx.atlassian.net/browse/TNL-6972 + return round_away_from_zero(grader_result['percent'] * 100 + 0.05) / 100 @staticmethod def _compute_letter_grade(grade_cutoffs, percent): diff --git a/lms/djangoapps/instructor/tests/test_spoc_gradebook.py b/lms/djangoapps/instructor/tests/test_spoc_gradebook.py index 078d753c66..1fa34bbf04 100644 --- a/lms/djangoapps/instructor/tests/test_spoc_gradebook.py +++ b/lms/djangoapps/instructor/tests/test_spoc_gradebook.py @@ -104,11 +104,11 @@ class TestDefaultGradingPolicy(TestGradebook): # Users 1-10 attempted any homework (and get Fs) [10] # Users 4-10 scored enough to not get rounded to 0 for the class (and get Fs) [7] # One use at top of the page [1] - self.assertEqual(22, self.response.content.count(b'grade_F')) + self.assertEqual(23, self.response.content.count(b'grade_F')) # All other grades are None [29 categories * 11 users - 27 non-empty grades = 292] # One use at the top of the page [1] - self.assertEqual(293, self.response.content.count(b'grade_None')) + self.assertEqual(292, self.response.content.count(b'grade_None')) class TestLetterCutoffPolicy(TestGradebook):