Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed subsection gating on course outline to consider rounded off g… #16837

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common/lib/xmodule/xmodule/graders.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ def grade(self, grade_sheet, generate_random_scores=False):
possible = scores[i].graded_total.possible
section_name = scores[i].display_name

percentage = earned / possible
percentage = scores[i].percent_graded
summary_format = u"{section_type} {index} - {name} - {percent:.0%} ({earned:.3n}/{possible:.3n})"
summary = summary_format.format(
index=i + self.starting_index,
Expand Down
19 changes: 12 additions & 7 deletions common/lib/xmodule/xmodule/tests/test_graders.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import ddt
from pytz import UTC
from lms.djangoapps.grades.scores import compute_percent
from xmodule import graders
from xmodule.graders import (
AggregatedScore, ProblemScore, ShowCorrectness, aggregate_scores
Expand Down Expand Up @@ -97,6 +98,10 @@ def __init__(self, graded_total, display_name):
self.graded_total = graded_total
self.display_name = display_name

@property
def percent_graded(self):
return compute_percent(self.graded_total.earned, self.graded_total.possible)

common_fields = dict(graded=True, first_attempted=datetime.now())
test_gradesheet = {
'Homework': {
Expand Down Expand Up @@ -153,11 +158,11 @@ def test_assignment_format_grader(self):
self.assertEqual(len(graded['section_breakdown']), 12 + 1)

graded = overflow_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.8880952380952382) # 100% + 10% / 5 assignments
self.assertAlmostEqual(graded['percent'], 0.8879999999999999) # 100% + 10% / 5 assignments
self.assertEqual(len(graded['section_breakdown']), 7 + 1)

graded = lab_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.9226190476190477)
self.assertAlmostEqual(graded['percent'], 0.92249999999999999)
self.assertEqual(len(graded['section_breakdown']), 7 + 1)

def test_assignment_format_grader_on_single_section_entry(self):
Expand All @@ -173,7 +178,7 @@ def test_assignment_format_grader_on_single_section_entry(self):
self.assertEqual(graded['section_breakdown'][0]['label'], 'Midterm')

graded = midterm_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.505)
self.assertAlmostEqual(graded['percent'], 0.50)
self.assertEqual(len(graded['section_breakdown']), 0 + 1)

def test_weighted_subsections_grader(self):
Expand Down Expand Up @@ -211,17 +216,17 @@ def test_weighted_subsections_grader(self):
empty_grader = graders.WeightedSubsectionsGrader([])

graded = weighted_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.5106547619047619)
self.assertAlmostEqual(graded['percent'], 0.50812499999999994)
self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1)
self.assertEqual(len(graded['grade_breakdown']), 3)

graded = over_one_weights_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.7688095238095238)
self.assertAlmostEqual(graded['percent'], 0.76624999999999999)
self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1)
self.assertEqual(len(graded['grade_breakdown']), 3)

graded = zero_weights_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.2525)
self.assertAlmostEqual(graded['percent'], 0.25)
self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1)
self.assertEqual(len(graded['grade_breakdown']), 3)

Expand Down Expand Up @@ -278,7 +283,7 @@ def test_grader_from_conf(self):
empty_grader = graders.grader_from_conf([])

graded = weighted_grader.grade(self.test_gradesheet)
self.assertAlmostEqual(graded['percent'], 0.5106547619047619)
self.assertAlmostEqual(graded['percent'], 0.50812499999999994)
self.assertEqual(len(graded['section_breakdown']), (12 + 1) + (7 + 1) + 1)
self.assertEqual(len(graded['grade_breakdown']), 3)

Expand Down
15 changes: 4 additions & 11 deletions lms/djangoapps/gating/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,7 @@ def _get_subsection_percentage(subsection_grade):
"""
Returns the percentage value of the given subsection_grade.
"""
return _calculate_ratio(subsection_grade.graded_total.earned, subsection_grade.graded_total.possible) * 100.0


def _calculate_ratio(earned, possible):
"""
Returns the percentage of the given earned and possible values.
"""
return float(earned) / float(possible) if possible else 0.0
return subsection_grade.percent_graded * 100.0


def evaluate_entrance_exam(course_grade, user):
Expand Down Expand Up @@ -109,8 +102,8 @@ def get_entrance_exam_score_ratio(course_grade, exam_chapter_key):
decimal value less than 1.
"""
try:
earned, possible = course_grade.score_for_chapter(exam_chapter_key)
entrance_exam_score_ratio = course_grade.chapter_percentage(exam_chapter_key)
except KeyError:
earned, possible = 0.0, 0.0
entrance_exam_score_ratio = 0.0, 0.0
log.warning(u'Gating: Unexpectedly failed to find chapter_grade for %s.', exam_chapter_key)
return _calculate_ratio(earned, possible)
return entrance_exam_score_ratio
7 changes: 4 additions & 3 deletions lms/djangoapps/grades/course_grade.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from .config import assume_zero_if_absent
from .subsection_grade import ZeroSubsectionGrade
from .subsection_grade_factory import SubsectionGradeFactory
from .scores import compute_percent


class CourseGradeBase(object):
Expand Down Expand Up @@ -108,9 +109,9 @@ def problem_scores(self):
problem_scores.update(subsection_grade.problem_scores)
return problem_scores

def score_for_chapter(self, chapter_key):
def chapter_percentage(self, chapter_key):
"""
Returns the aggregate weighted score for the given chapter.
Returns the rounded aggregate weighted percentage for the given chapter.
Raises:
KeyError if the chapter is not found.
"""
Expand All @@ -119,7 +120,7 @@ def score_for_chapter(self, chapter_key):
for section in chapter_grade['sections']:
earned += section.graded_total.earned
possible += section.graded_total.possible
return earned, possible
return compute_percent(earned, possible)

def score_for_module(self, location):
"""
Expand Down
12 changes: 12 additions & 0 deletions lms/djangoapps/grades/scores.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from openedx.core.lib.cache_utils import memoized
from xmodule.graders import ProblemScore
from numpy import around

from .transformer import GradesTransformer

Expand Down Expand Up @@ -143,6 +144,17 @@ def weighted_score(raw_earned, raw_possible, weight):
return float(raw_earned) * weight / raw_possible, float(weight)


def compute_percent(earned, possible):
"""
Returns the percentage of the given earned and possible values.
"""
if possible > 0:
# Rounds to two decimal places.
return around(earned / possible, decimals=2)
else:
return 0.0


def _get_score_from_submissions(submissions_scores, block):
"""
Returns the score values from the submissions API if found.
Expand Down
8 changes: 2 additions & 6 deletions lms/djangoapps/grades/subsection_grade.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@
from lazy import lazy

from lms.djangoapps.grades.models import BlockRecord, PersistentSubsectionGrade
from lms.djangoapps.grades.scores import get_score, possibly_scored
from lms.djangoapps.grades.scores import get_score, possibly_scored, compute_percent
from xmodule import block_metadata_utils, graders
from xmodule.graders import AggregatedScore, ShowCorrectness


log = getLogger(__name__)


Expand Down Expand Up @@ -141,10 +140,7 @@ def attempted_graded(self):

@property
def percent_graded(self):
if self.graded_total.possible > 0:
return self.graded_total.earned / self.graded_total.possible
else:
return 0.0
return compute_percent(self.graded_total.earned, self.graded_total.possible)

@staticmethod
def _compute_block_score(
Expand Down
13 changes: 9 additions & 4 deletions lms/djangoapps/grades/tests/test_subsection_grade.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@
from ..subsection_grade import CreateSubsectionGrade, ReadSubsectionGrade
from .utils import mock_get_score
from .base import GradeTestBase
from ddt import data, ddt, unpack


@ddt
class SubsectionGradeTest(GradeTestBase):
def test_create_and_read(self):
with mock_get_score(1, 2):

@data((50, 100, .50), (59.49, 100, .59), (59.51, 100, .60), (59.50, 100, .60), (60.5, 100, .60))
@unpack
def test_create_and_read(self, mock_earned, mock_possible, expected_result):
with mock_get_score(mock_earned, mock_possible):
# Create a grade that *isn't* saved to the database
created_grade = CreateSubsectionGrade(
self.sequence,
Expand All @@ -15,7 +20,7 @@ def test_create_and_read(self):
self.subsection_grade_factory._csm_scores,
)
self.assertEqual(PersistentSubsectionGrade.objects.count(), 0)
self.assertEqual(created_grade.percent_graded, 0.5)
self.assertEqual(created_grade.percent_graded, expected_result)

# save to db, and verify object is in database
created_grade.update_or_create_model(self.request.user)
Expand All @@ -35,7 +40,7 @@ def test_create_and_read(self):
self.assertEqual(created_grade.url_name, read_grade.url_name)
read_grade.all_total.first_attempted = created_grade.all_total.first_attempted = None
self.assertEqual(created_grade.all_total, read_grade.all_total)
self.assertEqual(created_grade.percent_graded, 0.5)
self.assertEqual(created_grade.percent_graded, expected_result)

def test_zero(self):
with mock_get_score(1, 0):
Expand Down
3 changes: 2 additions & 1 deletion lms/templates/courseware/progress.html
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ <h3 class="hd hd-3" id="chapter_${loop.index}">${ chapter['display_name']}</h3>
<%
earned = section.all_total.earned
total = section.all_total.possible
percentageString = "{0:.0%}".format( float(earned)/total) if earned > 0 and total > 0 else ""

percentageString = "{0:.0%}".format(section.percent_graded) if earned > 0 and total > 0 else ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: I wonder why earned needs to be > 0 here. Shouldn't it be earned >= 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nasthagiri section.percent_graded is performing a total > 0 check to see if calculating ratio is possible and returning 0.0 if it is not. And when calculating ratio is not possible we want empty string "" displayed instead of 0.0. For that purpose checking total > 0 should be enough and there doesn't seem to be a need for earned > 0 nor >= 0.

%>
<h4 class="hd hd-4">
<a href="${reverse('courseware_section', kwargs=dict(course_id=course.id.to_deprecated_string(), chapter=chapter['url_name'], section=section.url_name))}">
Expand Down