Moodle
  1. Moodle
  2. MDL-36955

Multianswer grading penalties do not address subparts

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.3
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Testing Instructions:
      Hide

      Before testing it is important to understand how multianswer question were graded before the patch and how they will be graded after this modification.
      Imagine the four parts question (each part default mark is 1 so question total default mark is 4 and penalty set to 33.33333%):
      Match the following cities with the correct state: * San Francisco: (California/Arizona) * Tucson: (California/Arizona) * Los Angeles: (California/Arizona) * Phoenix: (California/Arizona)
      And that on first try responses were California - Arizona - California - California
      and on the second try : California - Arizona- California - Arizona
      Before this change the resulting grade was 4 -0.3333333*4 = 2.67 because penalty was applied to the whole question even if only 1 subquestion was wrong on the first try.
      After this change the resulting grade will be 1 + 1 + 1 + (1 - 0.333333) = 3.67 because penalty will only be applied to the fourth part that was wrong on the first try
      I think that if students could vote on this tracker, this issue would have received many votes

      1. Create at least one multianswer (Cloze) question. You can use some examples for Moodle docs: http://docs.moodle.org/24/en/Cloze. It would be interesting that at least one of the created questions include subquestions with diffrents default marks to verify grading calculation is right in all cases
      2. Don't forget to add several hints to your question(s), for instance "This is the first hint", "This is second hint", ... To fully test this code it is necessary that the number of hint is not too low.
        The number of tries allowed in interactive with multiple tries is equal to the number of hints + 1. So if you create 3 hints you will have 4 tries allowed.
      3. Save and preview the question(s) setting "How questions behave" to "Interactive with multiple tries"
      4. As a first very easy test try to answer correctly to all subquestions but one on the first try and to all questions correctly on the second try. verify penalty is only applied to the subquestion that was wrong on the first try.
      5. Try to get different subquestions right at different tries (for instance 1 subquestion right at second try and another right at third try) and verify mark calculation is always OK
      6. Try the following combination on 2 subquestions a & b
        Try 1 : response a is right and response b is wrong
        Try 2 : response a is wrong and response b is right
        Try 3 : response a is right and response b is right
        And verify mark's calculation is done deducting 2 penalties for a and 1 for b
      7. Test all other combinations you can think of and verify calculation is done in all cases on a subquestion by subquestion basis and considering the last change of response on the subquestion
      Show
      Before testing it is important to understand how multianswer question were graded before the patch and how they will be graded after this modification. Imagine the four parts question (each part default mark is 1 so question total default mark is 4 and penalty set to 33.33333%): Match the following cities with the correct state: * San Francisco: (California/Arizona) * Tucson: (California/Arizona) * Los Angeles: (California/Arizona) * Phoenix: (California/Arizona) And that on first try responses were California - Arizona - California - California and on the second try : California - Arizona- California - Arizona Before this change the resulting grade was 4 -0.3333333*4 = 2.67 because penalty was applied to the whole question even if only 1 subquestion was wrong on the first try. After this change the resulting grade will be 1 + 1 + 1 + (1 - 0.333333) = 3.67 because penalty will only be applied to the fourth part that was wrong on the first try I think that if students could vote on this tracker, this issue would have received many votes Create at least one multianswer (Cloze) question. You can use some examples for Moodle docs: http://docs.moodle.org/24/en/Cloze . It would be interesting that at least one of the created questions include subquestions with diffrents default marks to verify grading calculation is right in all cases Don't forget to add several hints to your question(s), for instance "This is the first hint", "This is second hint", ... To fully test this code it is necessary that the number of hint is not too low. The number of tries allowed in interactive with multiple tries is equal to the number of hints + 1. So if you create 3 hints you will have 4 tries allowed. Save and preview the question(s) setting "How questions behave" to "Interactive with multiple tries" As a first very easy test try to answer correctly to all subquestions but one on the first try and to all questions correctly on the second try. verify penalty is only applied to the subquestion that was wrong on the first try. Try to get different subquestions right at different tries (for instance 1 subquestion right at second try and another right at third try) and verify mark calculation is always OK Try the following combination on 2 subquestions a & b Try 1 : response a is right and response b is wrong Try 2 : response a is wrong and response b is right Try 3 : response a is right and response b is right And verify mark's calculation is done deducting 2 penalties for a and 1 for b Test all other combinations you can think of and verify calculation is done in all cases on a subquestion by subquestion basis and considering the last change of response on the subquestion
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      46496

      Description

      Currently, the penalty system in the multianswer question affects the score of the entire question instead of just applying penalties to the subparts of the questions. For example, in a five part question with a 50% penalty, if a student gets just one of the five parts wrong, they will lose 50% of the total grade for that question. There is a patch that Tim wrote that adds a final grading policy that applies the penalty just to the subpart of the entire question. This tracker item is request that that patch becomes part of the core Moodle.
      See forum discussion and patch here: https://moodle.org/mod/forum/discuss.php?d=206894

        Issue Links

          Activity

          Hide
          Jean-Michel Vedrine added a comment -

          I created a git branch with Tim's code, but as he said now we need some tests.

          Show
          Jean-Michel Vedrine added a comment - I created a git branch with Tim's code, but as he said now we need some tests.
          Hide
          Emma Richardson added a comment -

          I have been running this patch on my live site for a whole semester. I have not seen any issues with it. Let me know what precise testing is needed and I would be happy to help.

          Show
          Emma Richardson added a comment - I have been running this patch on my live site for a whole semester. I have not seen any issues with it. Let me know what precise testing is needed and I would be happy to help.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Emma.
          Sorry if my comment was misleading : when I said "we need some tests" I was speaking of unit tests : some code that tests the function is really doing what we expect it to do. So I don't think you can help for that, even if the fact that this patch has been running on your site for a semester is very interesting because it shows that the code is working.
          I wrote the first test_compute_final_grade test but it is failing just because the computed grade 0.50000005 doesn't match the expected value 0.5 because of rounding errors in calculation. I need to look at other tests to see how Tim solve this rounding problem and write other tests.

          Show
          Jean-Michel Vedrine added a comment - Hello Emma. Sorry if my comment was misleading : when I said "we need some tests" I was speaking of unit tests : some code that tests the function is really doing what we expect it to do. So I don't think you can help for that, even if the fact that this patch has been running on your site for a semester is very interesting because it shows that the code is working. I wrote the first test_compute_final_grade test but it is failing just because the computed grade 0.50000005 doesn't match the expected value 0.5 because of rounding errors in calculation. I need to look at other tests to see how Tim solve this rounding problem and write other tests.
          Hide
          Jean-Michel Vedrine added a comment -

          Well in fact changing the penalty from 0.3333333 to 0.2 solve the rounding problem and the first test is OK. I will write some other tests.

          Show
          Jean-Michel Vedrine added a comment - Well in fact changing the penalty from 0.3333333 to 0.2 solve the rounding problem and the first test is OK. I will write some other tests.
          Hide
          Emma Richardson added a comment -

          That would explain why I have had no issues. We use .2 as the penalty!

          Show
          Emma Richardson added a comment - That would explain why I have had no issues. We use .2 as the penalty!
          Hide
          Tim Hunt added a comment -

          Look at check_current_mark in question/engine/tests/helpers.php. It does

          $this->assertEquals($mark, $this->quba->get_question_mark($this->slot),
          'Expected mark and actual mark differ.', 0.000001);

          That is, it compares marks with a tolerence of 1e-6.

          Show
          Tim Hunt added a comment - Look at check_current_mark in question/engine/tests/helpers.php. It does $this->assertEquals($mark, $this->quba->get_question_mark($this->slot), 'Expected mark and actual mark differ.', 0.000001); That is, it compares marks with a tolerence of 1e-6.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I have another problem. When subquestion number n is a multichoice one, $rightchoice = $question->subquestions[n]->get_correct_response(); and reset($rightchoice) permit me to be sure to submit a good response. But how can I be sure to submit a false (but existing) one ?

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I have another problem. When subquestion number n is a multichoice one, $rightchoice = $question->subquestions [n] ->get_correct_response(); and reset($rightchoice) permit me to be sure to submit a good response. But how can I be sure to submit a false (but existing) one ?
          Hide
          Tim Hunt added a comment -

          See test_interactive_feedback_multichoice_multiple_reset in question/behaviour/interactive/tests/walkthrough_test.php

          Show
          Tim Hunt added a comment - See test_interactive_feedback_multichoice_multiple_reset in question/behaviour/interactive/tests/walkthrough_test.php
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Tim,
          I found a solution before seeing your answer.
          Can you look at the first test I have done and somewhat review it before I go further ?

          Show
          Jean-Michel Vedrine added a comment - Thanks Tim, I found a solution before seeing your answer. Can you look at the first test I have done and somewhat review it before I go further ?
          Hide
          Tim Hunt added a comment -

          That looks good so far.

          Show
          Tim Hunt added a comment - That looks good so far.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Tim,
          Do you have ideas of other things I could test or interesting responses array combinations ?

          Show
          Jean-Michel Vedrine added a comment - Thanks Tim, Do you have ideas of other things I could test or interesting responses array combinations ?
          Hide
          Tim Hunt added a comment -

          I think just add one walkthrough tests. I cannot think of any specific things that might go wrong, what are worth testing for.

          Show
          Tim Hunt added a comment - I think just add one walkthrough tests. I cannot think of any specific things that might go wrong, what are worth testing for.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Thanks Tim,
          It will be in fact the first walkthrough test of the multianswer question type in interactive mode, it seems, as I discovered that qbehaviour_interactive_walkthrough_test don't include any multianswer question and that qtype_multianswer_walkthrough_test only test deferred behaviour.
          So I will do a walkthrough test for this issue and another one for MDL-28183.
          Should I put them in question/type/multianswer/tests/walkthrough_test.php or in question/behaviour/interactive/walkthrough_test.php ?

          Show
          Jean-Michel Vedrine added a comment - - edited Thanks Tim, It will be in fact the first walkthrough test of the multianswer question type in interactive mode, it seems, as I discovered that qbehaviour_interactive_walkthrough_test don't include any multianswer question and that qtype_multianswer_walkthrough_test only test deferred behaviour. So I will do a walkthrough test for this issue and another one for MDL-28183 . Should I put them in question/type/multianswer/tests/walkthrough_test.php or in question/behaviour/interactive/walkthrough_test.php ?
          Hide
          Tim Hunt added a comment -

          I think put them in question/type/multianswer/tests/walkthrough_test.php.

          Show
          Tim Hunt added a comment - I think put them in question/type/multianswer/tests/walkthrough_test.php.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello,
          I have added an interactive countback walkthrough test.
          In fact now that I have learned more helper functions while doing the one for MDL-28183 it was rather quick to do.
          But I lost an hour searching why the last step was not working and why my question was still in todo state, when I sudddently realized that the fix I did for the fourmc question in the helper was in my MDL-28183 branch but not in this one, and that my test was in fact working casting answer fractions to float solved the problem.

          Show
          Jean-Michel Vedrine added a comment - Hello, I have added an interactive countback walkthrough test. In fact now that I have learned more helper functions while doing the one for MDL-28183 it was rather quick to do. But I lost an hour searching why the last step was not working and why my question was still in todo state, when I sudddently realized that the fix I did for the fourmc question in the helper was in my MDL-28183 branch but not in this one, and that my test was in fact working casting answer fractions to float solved the problem.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim. Asking for the first peer review.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim. Asking for the first peer review.
          Hide
          Jean-Michel Vedrine added a comment -

          Sorry Tim, I forgot to add you as a watcher before requesting peer-review.

          Show
          Jean-Michel Vedrine added a comment - Sorry Tim, I forgot to add you as a watcher before requesting peer-review.
          Hide
          Tim Hunt added a comment -

          The code looks OK, but

          1. The main problem is that test_compute_final_grade is incomprehensible. What I mean is that it is not at all clear where the expected scores are coming from.

          So, the penalty is 0.2, and there are 2 parts to the question, so each half is worth 0.5 if you get it right first time, and each wrong answer costs you 0.1 of that.

          So, in the first test, they get Owl right on the second try, which should be 0.4, and sub2 right on the third try, which should be 0.3. Your code, however, asserts that $finalgrade is 2/3.

          The other test cases are equally baffling to me. How can the tests be passing if the expected grades are wrong?

          Also:

          2. Typo: test_iinteractivecountback_feedback

          3. Your try again step does not correctly clear the wrong responses, which it is supposed to, I think. Not a big deal, but worth getting write.

          Show
          Tim Hunt added a comment - The code looks OK, but 1. The main problem is that test_compute_final_grade is incomprehensible. What I mean is that it is not at all clear where the expected scores are coming from. So, the penalty is 0.2, and there are 2 parts to the question, so each half is worth 0.5 if you get it right first time, and each wrong answer costs you 0.1 of that. So, in the first test, they get Owl right on the second try, which should be 0.4, and sub2 right on the third try, which should be 0.3. Your code, however, asserts that $finalgrade is 2/3. The other test cases are equally baffling to me. How can the tests be passing if the expected grades are wrong? Also: 2. Typo: test_iinteractivecountback_feedback 3. Your try again step does not correctly clear the wrong responses, which it is supposed to, I think. Not a big deal, but worth getting write.
          Hide
          Tim Hunt added a comment -

          Sorry, I forgot.

          4. The other problem is that these changes will cause merge conflicts with MDL-28183. The normal thing to do in that case is to make the branch for this bug start from the MDL-28183 branch (you can do that with rebase). If you get it right, then the 'Master diff URL' for this bug should be https://github.com/jmvedrine/moodle/compare/MDL-28183...MDL-36955.

          Show
          Tim Hunt added a comment - Sorry, I forgot. 4. The other problem is that these changes will cause merge conflicts with MDL-28183 . The normal thing to do in that case is to make the branch for this bug start from the MDL-28183 branch (you can do that with rebase). If you get it right, then the 'Master diff URL' for this bug should be https://github.com/jmvedrine/moodle/compare/MDL-28183...MDL-36955 .
          Hide
          Jean-Michel Vedrine added a comment - - edited

          Hello Tim,
          1. For test_compute_final_grade test, as you say the question has 2 parts, but as I do $question->subquestions[2]->defaultmark = 2; the first part default mark is 1 and the second part default mark is 2.
          I did that because my thinking was that if each part's mark was the same, the test was not very demonstrative.
          When I write a test I first compute the grade myself manually to find what the grade should be in my opinion, then I write the asserts and when running the test if there is a fail, I look to see if my manual calculation was wrong or if my code doesn't really do what I was thinking it does. I must admit that both did happen
          That being said, you are right, the test is incomprehensible, maybe some comments explaining my intentions will help ?
          2. Oops will correct that
          3. In fact you are reviewing my tests in the reverse order I wrote them, and as you see I understood how clear wrong parts works after writing the tests for this issue and before finishing the tests for MDL-28183 I will correct the process_submission.

          Show
          Jean-Michel Vedrine added a comment - - edited Hello Tim, 1. For test_compute_final_grade test, as you say the question has 2 parts, but as I do $question->subquestions [2] ->defaultmark = 2; the first part default mark is 1 and the second part default mark is 2. I did that because my thinking was that if each part's mark was the same, the test was not very demonstrative. When I write a test I first compute the grade myself manually to find what the grade should be in my opinion, then I write the asserts and when running the test if there is a fail, I look to see if my manual calculation was wrong or if my code doesn't really do what I was thinking it does. I must admit that both did happen That being said, you are right, the test is incomprehensible, maybe some comments explaining my intentions will help ? 2. Oops will correct that 3. In fact you are reviewing my tests in the reverse order I wrote them, and as you see I understood how clear wrong parts works after writing the tests for this issue and before finishing the tests for MDL-28183 I will correct the process_submission.
          Hide
          Tim Hunt added a comment -

          1. To make it clearer, I suggest

          a. Add a comment pointing out that you have set $question->subquestions[2]->defaultmark = 2; to make it a better test, even thought (at the moment) that never happens for real.
          b. Write the expected grade like 1/3*0.8 + 2/3*0.6 (or even 1/3*(1 - 0.2) + 2/3*(1 - 2*0.2) if you think it is worth the extra effort).

          Show
          Tim Hunt added a comment - 1. To make it clearer, I suggest a. Add a comment pointing out that you have set $question->subquestions [2] ->defaultmark = 2; to make it a better test, even thought (at the moment) that never happens for real. b. Write the expected grade like 1/3*0.8 + 2/3*0.6 (or even 1/3*(1 - 0.2) + 2/3*(1 - 2*0.2) if you think it is worth the extra effort).
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I have made the fixes suggested by your first review.
          I have also rebased this branch on the one for MDL-28183 and updated the Master diff URL.
          It seems right to me, but as this is the first time I rebase on anything other than master (had quite some conflicts to resolve !!) better you look at it to see if I got it right
          I did run the units tests and the codechecker after rebase.

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I have made the fixes suggested by your first review. I have also rebased this branch on the one for MDL-28183 and updated the Master diff URL. It seems right to me, but as this is the first time I rebase on anything other than master (had quite some conflicts to resolve !!) better you look at it to see if I got it right I did run the units tests and the codechecker after rebase.
          Hide
          Jean-Michel Vedrine added a comment -

          Not yet squashed so that you can look at the last changes

          Show
          Jean-Michel Vedrine added a comment - Not yet squashed so that you can look at the last changes
          Hide
          Tim Hunt added a comment -

          Yay! now I can understand those tests.

          The rebasing looks right so far. (If you amend the commit comment for MDL-28183, then you will have to work out how to fix this branch )

          And I have now tested, and this seems to work fine.

          Show
          Tim Hunt added a comment - Yay! now I can understand those tests. The rebasing looks right so far. (If you amend the commit comment for MDL-28183 , then you will have to work out how to fix this branch ) And I have now tested, and this seems to work fine.
          Hide
          Jean-Michel Vedrine added a comment - - edited

          I have added testing instructions and also the doc_required label because we will need to edit http://docs.moodle.org/25/en/Cloze#Penalty_factor once Moodle docs for 2.5 version exists

          Show
          Jean-Michel Vedrine added a comment - - edited I have added testing instructions and also the doc_required label because we will need to edit http://docs.moodle.org/25/en/Cloze#Penalty_factor once Moodle docs for 2.5 version exists
          Hide
          Jean-Michel Vedrine added a comment -

          Hello TIm,
          Is anything missing so this can be send to integration ?

          Show
          Jean-Michel Vedrine added a comment - Hello TIm, Is anything missing so this can be send to integration ?
          Hide
          Tim Hunt added a comment -

          We were just missing one click on one button by me. Now done

          Show
          Tim Hunt added a comment - We were just missing one click on one button by me. Now done
          Hide
          Jean-Michel Vedrine added a comment -

          Wonderful Thanks Tim

          Show
          Jean-Michel Vedrine added a comment - Wonderful Thanks Tim
          Hide
          Jean-Michel Vedrine added a comment -

          Should we add a message to the integrators saying that this can only be integrated after MDL-28183 or is the git branch enough to tell them ?

          Show
          Jean-Michel Vedrine added a comment - Should we add a message to the integrators saying that this can only be integrated after MDL-28183 or is the git branch enough to tell them ?
          Hide
          Tim Hunt added a comment -

          To INTEGRATORS: this can only be integrated after MDL-28183.

          (Good point. It is better to make it explicit

          Show
          Tim Hunt added a comment - To INTEGRATORS: this can only be integrated after MDL-28183 . (Good point. It is better to make it explicit
          Hide
          Dan Poltawski added a comment -

          Integrated to master only, thanks Jean-Michel!

          Show
          Dan Poltawski added a comment - Integrated to master only, thanks Jean-Michel!
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Jean-Michel.

          Grades are calculated as described. I used example 1, with 5 answers and with 1 wrong answer I got 4.67/5, with 2 wrong answers it is 4.33/5.
          Calculates fine with multiple tries.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Jean-Michel. Grades are calculated as described. I used example 1, with 5 answers and with 1 wrong answer I got 4.67/5, with 2 wrong answers it is 4.33/5. Calculates fine with multiple tries.
          Hide
          Jean-Michel Vedrine added a comment -

          Perfect. Thanks Rajesh.

          Show
          Jean-Michel Vedrine added a comment - Perfect. Thanks Rajesh.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!
          Hide
          Emma Richardson added a comment -

          A huge thank you to you all for all of your hard work!!

          Show
          Emma Richardson added a comment - A huge thank you to you all for all of your hard work!!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: