Moodle
  1. Moodle
  2. MDL-13629

Gradebook: Unexpected behavior in selection of lowest grade to drop and resultant category calculation

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9, 1.9.12
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Gradebook
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      log in as admin or teacher.
      delete all gradeable activities (or create a new course)
      create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively.

      You may want to keep the grader report and categories and items "full view" open in 2 tabs.

      Go to the categories and items "full view" screen.
      Click the edit/hand icon for the course grade item.
      Set aggregation to "simple weighted mean of grades".
      Set maximum grade to 100.00
      Go to the grader report and enter a grade of 10 for the assignment out of 10

      On the categories screen, set "drop the lowest" to None.
      Tick "Aggregate only non-empty grades" if it isnt already
      On the grader report, the students course grade should become 100% --> (1.0*10)/10
      1.0 = the student grade, 100%
      the first 10 = the assignment maximum
      the other 10 = the sum of all included activity maximums
      (The student only has a single graded and they got 100% so their course grade is 100%)

      Untick "Aggregate only non-empty grades" for the course
      Reload the grader report.
      Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30
      (the other assignments are now included and are counted as 0)

      On the categories screen, set "drop the lowest" to 1.
      Reload the grader report.
      Course grade should become 66.7% (or 66.67 or similar) --> (0.0*5 + 1.0*10)/15
      (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment)

      Enter a 0 for the assignment out of 15.
      Reload the grader report.
      Course grade should stay at 66.67 --> (0.0*5 + 1.0*10)/15
      (it makes no difference if the assignment grade is 0 or empty)

      Enter a 0 for the assignment out of 5.
      The course grade should stay the same.

      Un-override the assignment out of 15 (it goes back to being empty)
      The course grade should stay the same.

      Un-override the assignment out of 5.
      The course grade should stay the same.

      Un-override the assignment out of 10.
      The course grade should go back to 0.

      Show
      log in as admin or teacher. delete all gradeable activities (or create a new course) create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively. You may want to keep the grader report and categories and items "full view" open in 2 tabs. Go to the categories and items "full view" screen. Click the edit/hand icon for the course grade item. Set aggregation to "simple weighted mean of grades". Set maximum grade to 100.00 Go to the grader report and enter a grade of 10 for the assignment out of 10 On the categories screen, set "drop the lowest" to None. Tick "Aggregate only non-empty grades" if it isnt already On the grader report, the students course grade should become 100% --> (1.0*10)/10 1.0 = the student grade, 100% the first 10 = the assignment maximum the other 10 = the sum of all included activity maximums (The student only has a single graded and they got 100% so their course grade is 100%) Untick "Aggregate only non-empty grades" for the course Reload the grader report. Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30 (the other assignments are now included and are counted as 0) On the categories screen, set "drop the lowest" to 1. Reload the grader report. Course grade should become 66.7% (or 66.67 or similar) --> (0.0*5 + 1.0*10)/15 (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment) Enter a 0 for the assignment out of 15. Reload the grader report. Course grade should stay at 66.67 --> (0.0*5 + 1.0*10)/15 (it makes no difference if the assignment grade is 0 or empty) Enter a 0 for the assignment out of 5. The course grade should stay the same. Un-override the assignment out of 15 (it goes back to being empty) The course grade should stay the same. Un-override the assignment out of 5. The course grade should stay the same. Un-override the assignment out of 10. The course grade should go back to 0.
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-13629_droplow
    • Rank:
      4180

      Description

      Using simple weighted mean of grade with assignments of varying point values I am not sure what the logic ought to be for selecting the lowest grade. I would assume that it would be the lowest percentage (points earned/points possible) for an assignment and where there are multiple zeros that it would select the one with the highest possible points (i.e. the one that would have the greatest weight).

      I would expect a blank and zero to be treated the same unless the gradebook as been set to ignore blank grades. In my particular case, I did not check the box for the category to Aggregate only non-empty grades. So I believe that an empty grade should be considered equivalent to a zero (at least that is the behavior I am expecting).

      What I noticed was that the category average changed depending on which assignment was blank and which was zero. I would have expected the category value to remain the same. I am attaching the course backup (zip) to facilitate reproduction as well as two screenshots showing the swap between the 0 and blank on the last student with different calculated values for the assignment category.

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          Just to be clear, in the screenshots - I'm looking at student Ignacio Jones where Assignment 3 and Assignment 4 are swapped between being 0 and blank. If 0 and blank are equivalent (as in this case they should be) then the calculated total for the assignments category should remain 20 and not turn to 50.

          Show
          Anthony Borrow added a comment - Just to be clear, in the screenshots - I'm looking at student Ignacio Jones where Assignment 3 and Assignment 4 are swapped between being 0 and blank. If 0 and blank are equivalent (as in this case they should be) then the calculated total for the assignments category should remain 20 and not turn to 50.
          Hide
          Petr Škoda added a comment -

          blank grade is treated as minimal grade,
          the limiting rules are applied after converting grade values to percentages

          I think it works fine

          Show
          Petr Škoda added a comment - blank grade is treated as minimal grade, the limiting rules are applied after converting grade values to percentages I think it works fine
          Hide
          Anthony Borrow added a comment -

          Petr - dropping blanks has Ignacio with two blank scores. Which one should be dropped? I think in the documentation we may need to explain this logic. Do we drop the first one in the list or the one worth the greatest number of points? It looks like it may be the first in the list but I'd have to do more testing to confirm. Peace - Anthony

          Show
          Anthony Borrow added a comment - Petr - dropping blanks has Ignacio with two blank scores. Which one should be dropped? I think in the documentation we may need to explain this logic. Do we drop the first one in the list or the one worth the greatest number of points? It looks like it may be the first in the list but I'd have to do more testing to confirm. Peace - Anthony
          Hide
          Anthony Borrow added a comment - - edited

          Petr - In the blankmin.png file, Ignacio's scores do not seem to follow your rule that the blank is dropped. If it were I would expect a higher score (namely 50). Peace - Anthony

          Show
          Anthony Borrow added a comment - - edited Petr - In the blankmin.png file, Ignacio's scores do not seem to follow your rule that the blank is dropped. If it were I would expect a higher score (namely 50). Peace - Anthony
          Hide
          Petr Škoda added a comment -

          hmm, yes you are right - when dropping lowest/highes the values are ordered by percentage value and if several values are equal the selection is random (which is not a problem in other aggregation types)

          solution could be:
          1/ disable these options in this type of aggregation
          2/ or rewrite the code - add special treatment of weighted means

          Show
          Petr Škoda added a comment - hmm, yes you are right - when dropping lowest/highes the values are ordered by percentage value and if several values are equal the selection is random (which is not a problem in other aggregation types) solution could be: 1/ disable these options in this type of aggregation 2/ or rewrite the code - add special treatment of weighted means
          Hide
          Petr Škoda added a comment -

          please test the attached patch, it might do what you want

          Show
          Petr Škoda added a comment - please test the attached patch, it might do what you want
          Hide
          Petr Škoda added a comment -

          Thinking about this a bit more, I find it problematic to define the "lowest" grade, it can be:
          a/ the lowest entered number (in patch above)
          b/ the lowest percentage value (in cvs: grademin == 0% grademax == 100%)
          c) one of those above affected by item weight too

          Neither above returns easy to guess results imho and it gets even more complicated if normal weighted mean used.

          Show
          Petr Škoda added a comment - Thinking about this a bit more, I find it problematic to define the "lowest" grade, it can be: a/ the lowest entered number (in patch above) b/ the lowest percentage value (in cvs: grademin == 0% grademax == 100%) c) one of those above affected by item weight too Neither above returns easy to guess results imho and it gets even more complicated if normal weighted mean used.
          Hide
          Anthony Borrow added a comment -

          I'll evaluate and play with the patch and look at the code when I get a chance. I would say that I think I would stick with the CVS behavior (i.e. lowest percentage value). I'm not clear how item weight plays into this but we can deal with this as a fix for 1.9.1.

          Show
          Anthony Borrow added a comment - I'll evaluate and play with the patch and look at the code when I get a chance. I would say that I think I would stick with the CVS behavior (i.e. lowest percentage value). I'm not clear how item weight plays into this but we can deal with this as a fix for 1.9.1.
          Hide
          Michael de Raadt added a comment -

          Increasing the priority of this issue as it has now been duplicated twice.

          Show
          Michael de Raadt added a comment - Increasing the priority of this issue as it has now been duplicated twice.
          Hide
          Andrew Davis added a comment - - edited

          Im going to take a look at this. The bug fixing period at Moodle HQ for Moodle 1.9 has finished. At a minimum I'll need to verify whether or not this is still a problem in more recent versions of Moodle.

          I am attempting to estimate my future work so that I can manage my time better. My current estimate for determing if this is still an issue is 2 hours. Once Ive made that determination I'll have a better idea of how long it will take to get this fixed, integrated and tested.

          Show
          Andrew Davis added a comment - - edited Im going to take a look at this. The bug fixing period at Moodle HQ for Moodle 1.9 has finished. At a minimum I'll need to verify whether or not this is still a problem in more recent versions of Moodle. I am attempting to estimate my future work so that I can manage my time better. My current estimate for determing if this is still an issue is 2 hours. Once Ive made that determination I'll have a better idea of how long it will take to get this fixed, integrated and tested.
          Hide
          Anthony Borrow added a comment -

          Thanks for verifying this is still an issue in 2.x. Let me know if you need any help testing. Peace - Anthony

          Show
          Anthony Borrow added a comment - Thanks for verifying this is still an issue in 2.x. Let me know if you need any help testing. Peace - Anthony
          Hide
          Andrew Davis added a comment -

          It appears that this is still an issue. My testing notes are below. Ill eventually use these as inspiration for the testing intructions if you're wondering why they're phrased as they are.

          log in as admin or teacher.
          delete all gradeable activities (or create a new course)
          create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively.

          You may want to keep the grader report and categories and items "full view" open in 2 tabs.

          Go to the categories and items "full view" screen.
          Click the edit/hand icon for the course grade item.
          Set aggregation to "simple weighted mean of grades".
          Set maximum grade to 100.00
          Go to the grader report and enter a grade of 10 for the assignment out of 10

          On the categories screen, set "drop the lowest" to None.
          Tick "Aggregate only non-empty grades" if it isnt already
          On the grader report, the students course grade should become 100% --> (1.0*10)/10
          1.0 = the student grade, 100%
          the first 10 = the assignment maximum
          the other 10 = the sum of all included activity maximums
          (The student only has a single graded and they got 100% so their course grade is 100%)

          Untick "Aggregate only non-empty grades" for the course
          Reload the grader report.
          Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30
          (the other assignments are now included and are counted as 0.

          On the categories screen, set "drop the lowest" to 1.
          Reload the grader report.
          Course grade should become 66.7% --> (0.0*5 + 1.0*10)/15
          (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment)

          Enter a 0 for the assignment out of 15.
          Reload the grader report.
          Course grade becomes 40.0% --> (1.0*10 + 0.0*15)/25
          (now the assignment out of 5 is being removed)

          Enter a 0 for the assignment out of 5.
          Un-override the assignment out of 15 (it goes back to being empty)
          Reload the grader report.
          Course grade becomes 66.67.0% --> (0.0*5 + 1.0*10)/15
          (now the assignment out of 15 is again being removed. It appears that empty grades are dropped in preference to 0s. I'm not sure this is correct. It seems like we should be considering 0s and empties equal, assuming the minimum grade is 0, and thus still be dropping the most "weighty" assignment)

          Show
          Andrew Davis added a comment - It appears that this is still an issue. My testing notes are below. Ill eventually use these as inspiration for the testing intructions if you're wondering why they're phrased as they are. log in as admin or teacher. delete all gradeable activities (or create a new course) create 3 assignments. use "simple direct grading". Set them to have a maximum grade of 5, 10 and 15 respectively. You may want to keep the grader report and categories and items "full view" open in 2 tabs. Go to the categories and items "full view" screen. Click the edit/hand icon for the course grade item. Set aggregation to "simple weighted mean of grades". Set maximum grade to 100.00 Go to the grader report and enter a grade of 10 for the assignment out of 10 On the categories screen, set "drop the lowest" to None. Tick "Aggregate only non-empty grades" if it isnt already On the grader report, the students course grade should become 100% --> (1.0*10)/10 1.0 = the student grade, 100% the first 10 = the assignment maximum the other 10 = the sum of all included activity maximums (The student only has a single graded and they got 100% so their course grade is 100%) Untick "Aggregate only non-empty grades" for the course Reload the grader report. Course grade should become 33.33%--> (0.0*5 + 1.0*10 + 0.0*15)/30 (the other assignments are now included and are counted as 0. On the categories screen, set "drop the lowest" to 1. Reload the grader report. Course grade should become 66.7% --> (0.0*5 + 1.0*10)/15 (the denominator was reduced due to the removal of the assignment out of 15, the most "weighty" assignment) Enter a 0 for the assignment out of 15. Reload the grader report. Course grade becomes 40.0% --> (1.0*10 + 0.0*15)/25 (now the assignment out of 5 is being removed) Enter a 0 for the assignment out of 5. Un-override the assignment out of 15 (it goes back to being empty) Reload the grader report. Course grade becomes 66.67.0% --> (0.0*5 + 1.0*10)/15 (now the assignment out of 15 is again being removed. It appears that empty grades are dropped in preference to 0s. I'm not sure this is correct. It seems like we should be considering 0s and empties equal, assuming the minimum grade is 0, and thus still be dropping the most "weighty" assignment)
          Hide
          Andrew Davis added a comment -

          In lib/grade/tests/grade_category_test.php is a test method sub_test_grade_category_apply_limit_rules() that tests grade category "droplow" and "keephigh". I'm now attempting to expand the test set to include the reproduction of this bug. If I can manage that I can be confident of any subsequent fix and can ensure this bug doesn't come up again in future.

          Show
          Andrew Davis added a comment - In lib/grade/tests/grade_category_test.php is a test method sub_test_grade_category_apply_limit_rules() that tests grade category "droplow" and "keephigh". I'm now attempting to expand the test set to include the reproduction of this bug. If I can manage that I can be confident of any subsequent fix and can ensure this bug doesn't come up again in future.
          Hide
          Andrew Davis added a comment -

          Im adding a git branch that contains enhanced unit tests. I've added a new test that reproduces the problem. There are two grade items with a 0 grade. One has a maxgrade of 100, the other has a maxgrade of 110. Currently the grade item with a maxgrade of 100 is being excluded by droplow which is incorrect. This unit test will fail until apply_limit_rules() is fixed which is my next task.

          Show
          Andrew Davis added a comment - Im adding a git branch that contains enhanced unit tests. I've added a new test that reproduces the problem. There are two grade items with a 0 grade. One has a maxgrade of 100, the other has a maxgrade of 110. Currently the grade item with a maxgrade of 100 is being excluded by droplow which is incorrect. This unit test will fail until apply_limit_rules() is fixed which is my next task.
          Hide
          Andrew Davis added a comment -

          Ok, I think this is all done. I've made apply_limit_rules() a bit smarter about which grade items it drops when droplow is used. Putting this up for peer review.

          Show
          Andrew Davis added a comment - Ok, I think this is all done. I've made apply_limit_rules() a bit smarter about which grade items it drops when droplow is used. Putting this up for peer review.
          Hide
          David Monllaó added a comment -

          Hi Andrew,

          I see that grade_values is ordered by the value, but the order of the elements with the same value will not be always the same, so checking only the next item grademax could not be enough to be sure that we always skip the one with the same value and the highest grademax.

          I tested your scenario with 3 different users on the course, the dropped items differs for every student and the course final grade also differs.

          Thanks

          Show
          David Monllaó added a comment - Hi Andrew, I see that grade_values is ordered by the value, but the order of the elements with the same value will not be always the same, so checking only the next item grademax could not be enough to be sure that we always skip the one with the same value and the highest grademax. I tested your scenario with 3 different users on the course, the dropped items differs for every student and the course final grade also differs. Thanks
          Hide
          Andrew Davis added a comment - - edited

          Thanks David. Maybe I'm going about this the wrong way but its a deceptively complex little problem. I think I have it now. I'll take another look in the morning.

          Show
          Andrew Davis added a comment - - edited Thanks David. Maybe I'm going about this the wrong way but its a deceptively complex little problem. I think I have it now. I'll take another look in the morning.
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Andrew Davis added a comment -

          Putting this up for peer review.

          Show
          Andrew Davis added a comment - Putting this up for peer review.
          Hide
          David Monllaó added a comment -

          Sorry Andrew, it looks ok with the extra while, but testing the same scenario I've noticed the same issue, it seems that is here https://github.com/andyjdavis/moodle/compare/master...MDL-13629_droplow#L0R915, the assigned value is a float and the value of the empty grades item comes as an integer

          Show
          David Monllaó added a comment - Sorry Andrew, it looks ok with the extra while, but testing the same scenario I've noticed the same issue, it seems that is here https://github.com/andyjdavis/moodle/compare/master...MDL-13629_droplow#L0R915 , the assigned value is a float and the value of the empty grades item comes as an integer
          Hide
          Andrew Davis added a comment -

          Added testing instructions and an updated fix.

          Show
          Andrew Davis added a comment - Added testing instructions and an updated fix.
          Hide
          David Monllaó added a comment -

          It looks good and passes the test, thanks Andrew. Finishing peer review

          Show
          David Monllaó added a comment - It looks good and passes the test, thanks Andrew. Finishing peer review
          Hide
          Andrew Davis added a comment -

          Created the various branches. Putting this up for integration.

          Show
          Andrew Davis added a comment - Created the various branches. Putting this up for integration.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi Andrew,

          Taken some time trying to get my head around all this, its quite complex logic going on within loops. Great to see unit tests for this.

          The thing i'm finding difficult to understand is the purpose of the second while loop:
          while ($originalindex+$i < count($grade_keys))

          I just don't really know whats going on in there, so I was wondering if you could add a comment to the code to explain whats going on there?

          Show
          Dan Poltawski added a comment - Hi Andrew, Taken some time trying to get my head around all this, its quite complex logic going on within loops. Great to see unit tests for this. The thing i'm finding difficult to understand is the purpose of the second while loop: while ($originalindex+$i < count($grade_keys)) I just don't really know whats going on in there, so I was wondering if you could add a comment to the code to explain whats going on there?
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Andrew Davis added a comment -

          I've added an explanatory comment that hopefully makes it more clear.

          The goal is to drop n grade items with the lowest grade values (the grade the student has obtained). Extra credit grade items are never dropped. Where multiple grade items have the same grade value, for example 0, we need to drop the grade items with the highest grademax.

          Show
          Andrew Davis added a comment - I've added an explanatory comment that hopefully makes it more clear. The goal is to drop n grade items with the lowest grade values (the grade the student has obtained). Extra credit grade items are never dropped. Where multiple grade items have the same grade value, for example 0, we need to drop the grade items with the highest grademax.
          Hide
          Dan Poltawski added a comment -

          Thanks Andrew. I'll try to integrate and test this myself today because I don't think we have the capacity for anyone else to take this testing on.

          Show
          Dan Poltawski added a comment - Thanks Andrew. I'll try to integrate and test this myself today because I don't think we have the capacity for anyone else to take this testing on.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Andrew.

          I've tested 22 and master during integration review. Going to test on 23 now.

          Show
          Dan Poltawski added a comment - Integrated, thanks Andrew. I've tested 22 and master during integration review. Going to test on 23 now.
          Hide
          Dan Poltawski added a comment -

          Looks good on 2.3 too. Test passed, thanks Andrew.

          Show
          Dan Poltawski added a comment - Looks good on 2.3 too. Test passed, thanks Andrew.
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: