Revert "Update answer-pool within Jeffs 3x"

This reverts commit e639738e.

CHANGELOG.rst

@@ -188,7 +188,7 @@ duplicated "\index".
 Studio: Support answer pools for multiple choice question choices, so authors can provide
 multiple incorrect and correct choices for a question and have 1 correct choice and n-1
 incorrect choices randomly selected and shuffled before being presented to the student.
-In XML: <choicegroup answer-pool="4"> enables an answer pool of 4 choices: 3
+In XML: <multiplechoiceresponse answer-pool="4"> enables an answer pool of 4 choices: 3
 correct choices and 1 incorrect choice. To provide multiple solution expanations, wrap
 all solution elements within a <solutionset>, and make sure to add an attribute called
 "explanation-id" to both the <solution> tag and its corresponding <choice> tag, and be

common/lib/capa/capa/capa_problem.py

@@ -383,92 +383,103 @@ class LoncapaProblem(object):
             answer_ids.append(results.keys())
         return answer_ids
 
-    def sample_from_answer_pool(self, choices, rnd, num_pool):
+    def sample_from_answer_pool(self, choices, rnd, num_choices):
         """
         Takes in:
             1. list of choices
             2. random number generator
-            3. the requested size "answer-pool" number, in effect a max
+            3. max number of total choices to return
 
-        Returns a tuple with 2 items:
+        Returns a list with 2 items:
             1. the solution_id corresponding with the chosen correct answer
-            2. (subset) list of choice nodes with num-1 incorrect and 1 correct
+            2. (subset) list of choice nodes with 3 incorrect and 1 correct
         """
 
         correct_choices = []
         incorrect_choices = []
+        subset_choices = []
 
         for choice in choices:
             if choice.get('correct') == 'true':
                 correct_choices.append(choice)
-            else:
+            elif choice.get('correct') == 'false':
                 incorrect_choices.append(choice)
-                # TODO: check if we should require correct == "false"
 
-        # We throw an error if the problem is highly ill-formed.
-        # There must be at least one correct and one incorrect choice.
-        # TODO: perhaps this makes more sense for *all* problems, not just down in this corner.
-        if len(correct_choices) < 1 or len(incorrect_choices) < 1:
-            raise responsetypes.LoncapaProblemError("Choicegroup must include at last 1 correct and 1 incorrect choice")
+        # Always 1 correct and num_choices at least as large as this; if not, return list with no choices
+        num_correct = 1
+        if len(correct_choices) < num_correct or num_choices < num_correct:
+            return []
 
-        # Limit the number of incorrect choices to what we actually have
-        num_incorrect = num_pool - 1
+        # Ensure number of incorrect choices is no more than the number of incorrect choices to choose from
+        num_incorrect = num_choices - num_correct
         num_incorrect = min(num_incorrect, len(incorrect_choices))
 
-        # Select the one correct choice
+        # Use rnd given to us to generate a random number (see details in tree_using_answer_pool method)
         index = rnd.randint(0, len(correct_choices) - 1)
         correct_choice = correct_choices[index]
+        subset_choices.append(correct_choice)
         solution_id = correct_choice.get('explanation-id')
 
-        # Put together the result, pushing most of the work onto rnd.shuffle() 
-        subset_choices = [correct_choice]
-        rnd.shuffle(incorrect_choices)
-        subset_choices += incorrect_choices[:num_incorrect]
-        rnd.shuffle(subset_choices)
-
-        return (solution_id, subset_choices)
-
-    def do_answer_pool(self, tree):
+        # Add incorrect choices
+        to_add = num_incorrect
+        while to_add > 0:
+            index = rnd.randint(0, len(incorrect_choices) - 1)
+            choice = incorrect_choices[index]
+            subset_choices.append(choice)
+            incorrect_choices.remove(choice)
+            to_add = to_add - 1
+
+        # Randomize correct answer position
+        index = rnd.randint(0, num_incorrect)
+        if index != 0:
+            tmp = subset_choices[index]
+            subset_choices[index] = subset_choices[0]  # where we put the correct answer
+            subset_choices[0] = tmp
+
+        return [solution_id, subset_choices]
+
+    def tree_using_answer_pool(self, tree):
         """
-        Implements the answer-pool subsetting operation in-place on the tree.
         Allows for problem questions with a pool of answers, from which answer options shown to the student
         and randomly selected so that there is always 1 correct answer and n-1 incorrect answers,
-        where the author specifies n as the value of the attribute "answer-pool" within <choicegroup>
+        where the user specifies n as the value of the attribute "answer-pool" within <multiplechoiceresponse>
+
+        The <multiplechoiceresponse> tag must have an attribute 'answer-pool' with integer value of n
+          - if so, this method will modify the tree
+          - if not, this method will not modify the tree
 
-        The <choicegroup> tag must have an attribute 'answer-pool' giving the desired
-        pool size. If that attribute is zero or not present, no operation is performed.
-        Calling this a second time does nothing.
+        These problems are colloquially known as "Gradiance" problems.
         """
 
-        # If called a second time, don't do anything, since it's in-place destructive
-        if hasattr(self, 'answerpool_done'):
+        query = '//multiplechoiceresponse[@answer-pool]'
+
+        # There are no questions with an answer pool
+        if not tree.xpath(query):
             return
-        self.answerpool_done = True
-        
-        choicegroups = tree.xpath("//choicegroup[@answer-pool]")
 
         # Uses self.seed -- but want to randomize every time reaches this problem,
         # so problem's "randomization" should be set to "always"
         rnd = Random(self.seed)
 
-        for choicegroup in choicegroups:
-            num_str = choicegroup.get('answer-pool')
-            try:
-                num_choices = int(num_str)
-            except ValueError:
-                raise responsetypes.LoncapaProblemError("answer-pool value should be an integer")
-            # choices == 0 disables the feature
-            if num_choices == 0:
-                break
+        for mult_choice_response in tree.xpath(query):
+            # Determine number of choices to display; if invalid number of choices, skip over
+            num_choices = mult_choice_response.get('answer-pool')
+            if not num_choices.isdigit():
+                continue
+            num_choices = int(num_choices)
+            if num_choices < 1:
+                continue
 
-            choices_list = list(choicegroup.getchildren())
+            # Grab the first choicegroup (there should only be one within each <multiplechoiceresponse> tag)
+            choicegroup = mult_choice_response.xpath('./choicegroup[@type="MultipleChoice"]')[0]
+            choices_list = list(choicegroup.iter('choice'))
 
             # Remove all choices in the choices_list (we will add some back in later)
             for choice in choices_list:
                 choicegroup.remove(choice)
 
             # Sample from the answer pool to get the subset choices and solution id
-            (solution_id, subset_choices) = self.sample_from_answer_pool(choices_list, rnd, num_choices)
+            [solution_id, subset_choices] = self.sample_from_answer_pool(choices_list, rnd, num_choices)
 
             # Add back in randomly selected choices
             for choice in subset_choices:
@@ -476,7 +487,7 @@ class LoncapaProblem(object):
 
             # Filter out solutions that don't correspond to the correct answer we selected to show
             # Note that this means that if the user simply provides a <solution> tag, nothing is filtered
-            solutionset = choicegroup.xpath('../following-sibling::solutionset')
+            solutionset = mult_choice_response.xpath('./following-sibling::solutionset')
             if len(solutionset) != 0:
                 solutionset = solutionset[0]
                 solutions = solutionset.xpath('./solution')
@@ -484,7 +495,7 @@ class LoncapaProblem(object):
                     if solution.get('explanation-id') != solution_id:
                         solutionset.remove(solution)
 
-    def do_targeted_feedback(self, tree):
+    def tree_using_targeted_feedback(self, tree):
         """
 Allows for problem questions to show targeted feedback, which are choice-level explanations.
 Targeted feedback is automatically visible after a student has submitted their answers.
@@ -500,11 +511,7 @@ the "Show Answer" setting to "Never" because now there's no need for a "Show Ans
 button because no solution will show up if you were to click the "Show Answer" button
 """
 
-        # If called a second time, don't do anything, since it's in-place destructive
-        if hasattr(self, 'targeted_done'):
-            return
-        self.targeted_done = True
-
+        # Note that if there are no questions with targeted feedback, the body of the for loop is not executed
         for mult_choice_response in tree.xpath('//multiplechoiceresponse[@targeted-feedback]'):
             show_explanation = mult_choice_response.get('targeted-feedback') == 'alwaysShowCorrectChoiceExplanation'
 
@@ -571,8 +578,8 @@ button because no solution will show up if you were to click the "Show Answer" b
         '''
         Main method called externally to get the HTML to be rendered for this capa Problem.
         '''
-        self.do_answer_pool(self.tree)
-        self.do_targeted_feedback(self.tree)
+        self.tree_using_answer_pool(self.tree)
+        self.tree_using_targeted_feedback(self.tree)
         html = contextualize_text(etree.tostring(self._extract_html(self.tree)), self.context)
 
         return html

common/lib/capa/capa/tests/test_answer_pool.py

 """
-Tests the logic of the "answer-pool" attribute, e.g.
-  <choicegroup answer-pool="4"
+Tests the logic of the "answer-pool" attribute for MultipleChoice questions,
+i.e. those with the <multiplechoiceresponse> element
 """
 
 import unittest
 import textwrap
 from . import test_system, new_loncapa_problem
-from capa.responsetypes import LoncapaProblemError
-
 
 
 class CapaAnswerPoolTest(unittest.TestCase):
@@ -24,8 +22,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+            <multiplechoiceresponse answer-pool="4">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -59,20 +57,16 @@ class CapaAnswerPoolTest(unittest.TestCase):
         problem = new_loncapa_problem(xml_str)
         problem.seed = 723
         the_html = problem.get_html()
-        # [('choice_3', u'wrong-3'), ('choice_5', u'correct-2'), ('choice_1', u'wrong-2'), ('choice_4', u'wrong-4')]
-        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-3'.*'correct-2'.*'wrong-2'.*'wrong-4'.*\].*</div>")
+        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-3'.*'wrong-1'.*'wrong-2'.*'correct-2'.*\].*</div>")
         self.assertRegexpMatches(the_html, r"<div>\{.*'1_solution_2'.*\}</div>")
-        # Calling get_html multiple times should yield the same thing
-        the_html2 = problem.get_html()
-        self.assertEquals(the_html, the_html2)
 
     def test_answer_pool_4_choices_1_multiplechoiceresponse_seed2(self):
         xml_str = textwrap.dedent("""
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+            <multiplechoiceresponse answer-pool="4">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -106,8 +100,7 @@ class CapaAnswerPoolTest(unittest.TestCase):
         problem = new_loncapa_problem(xml_str)
         problem.seed = 9
         the_html = problem.get_html()
-        # [('choice_0', u'wrong-1'), ('choice_4', u'wrong-4'), ('choice_3', u'wrong-3'), ('choice_2', u'correct-1')]
-        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-1'.*'wrong-4'.*'wrong-3'.*'correct-1'.*\].*</div>")
+        self.assertRegexpMatches(the_html, r"<div>.*\[.*'correct-1'.*'wrong-2'.*'wrong-1'.*'wrong-4'.*\].*</div>")
         self.assertRegexpMatches(the_html, r"<div>\{.*'1_solution_1'.*\}</div>")
 
     def test_no_answer_pool_4_choices_1_multiplechoiceresponse(self):
@@ -157,8 +150,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="0">
+            <multiplechoiceresponse answer-pool="0">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -194,13 +187,13 @@ class CapaAnswerPoolTest(unittest.TestCase):
         self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-1'.*'wrong-2'.*'correct-1'.*'wrong-3'.*'wrong-4'.*'correct-2'.*\].*</div>")
         self.assertRegexpMatches(the_html, r"<div>\{.*'1_solution_1'.*'1_solution_2'.*\}</div>")
 
-    def test_invalid_answer_pool(self):
+    def test_invalid_answer_pool_4_choices_1_multiplechoiceresponse(self):
         xml_str = textwrap.dedent("""
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="2.3">
+            <multiplechoiceresponse answer-pool="2.3">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -232,16 +225,17 @@ class CapaAnswerPoolTest(unittest.TestCase):
         """)
 
         problem = new_loncapa_problem(xml_str)
-        with self.assertRaises(LoncapaProblemError):
-            the_html = problem.get_html()
+        the_html = problem.get_html()
+        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-1'.*'wrong-2'.*'correct-1'.*'wrong-3'.*'wrong-4'.*'correct-2'.*\].*</div>")
+        self.assertRegexpMatches(the_html, r"<div>\{.*'1_solution_1'.*'1_solution_2'.*\}</div>")
 
     def test_answer_pool_5_choices_1_multiplechoiceresponse_seed1(self):
         xml_str = textwrap.dedent("""
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="5">
+            <multiplechoiceresponse answer-pool="5">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -275,7 +269,7 @@ class CapaAnswerPoolTest(unittest.TestCase):
         problem = new_loncapa_problem(xml_str)
         problem.seed = 723
         the_html = problem.get_html()
-        self.assertRegexpMatches(the_html, r"<div>.*\[.*'correct-2'.*'wrong-1'.*'wrong-2'.*.*'wrong-3'.*'wrong-4'.*\].*</div>")
+        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-2'.*'wrong-1'.*'correct-2'.*'wrong-3'.*'wrong-4'.*\].*</div>")
         self.assertRegexpMatches(the_html, r"<div>\{.*'1_solution_2'.*\}</div>")
 
     def test_answer_pool_2_multiplechoiceresponses_seed1(self):
@@ -283,8 +277,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+            <multiplechoiceresponse answer-pool="4">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -312,8 +306,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             </solutionset>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="3">
+            <multiplechoiceresponse answer-pool="3">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -348,8 +342,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
         problem.seed = 723
         the_html = problem.get_html()
 
-        str1 = r"<div>.*\[.*'wrong-3'.*'correct-2'.*'wrong-2'.*'wrong-4'.*\].*</div>"
-        str2 = r"<div>.*\[.*'wrong-2'.*'wrong-1'.*'correct-2'.*\].*</div>"
+        str1 = r"<div>.*\[.*'wrong-3'.*'wrong-1'.*'wrong-2'.*'correct-2'.*\].*</div>"
+        str2 = r"<div>.*\[.*'wrong-4'.*'wrong-2'.*'correct-2'.*\].*</div>"
         str3 = r"<div>\{.*'1_solution_2'.*\}</div>"
         str4 = r"<div>\{.*'1_solution_4'.*\}</div>"
 
@@ -368,8 +362,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="3">
+            <multiplechoiceresponse answer-pool="3">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -397,8 +391,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             </solutionset>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+            <multiplechoiceresponse answer-pool="4">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -433,10 +427,10 @@ class CapaAnswerPoolTest(unittest.TestCase):
         problem.seed = 9
         the_html = problem.get_html()
 
-        str1 = r"<div>.*\[.*'wrong-4'.*'wrong-3'.*'correct-1'.*\].*</div>"
-        str2 = r"<div>.*\[.*'wrong-2'.*'wrong-3'.*'wrong-4'.*'correct-2'.*\].*</div>"
+        str1 = r"<div>.*\[.*'wrong-1'.*'wrong-2'.*'correct-1'.*\].*</div>"
+        str2 = r"<div>.*\[.*'wrong-4'.*'wrong-3'.*'correct-1'.*'wrong-1'.*\].*</div>"
         str3 = r"<div>\{.*'1_solution_1'.*\}</div>"
-        str4 = r"<div>\{.*'1_solution_4'.*\}</div>"
+        str4 = r"<div>\{.*'1_solution_3'.*\}</div>"
 
         self.assertRegexpMatches(the_html, str1)
         self.assertRegexpMatches(the_html, str2)
@@ -472,8 +466,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             </solution>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+            <multiplechoiceresponse answer-pool="4">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -509,7 +503,7 @@ class CapaAnswerPoolTest(unittest.TestCase):
         the_html = problem.get_html()
 
         str1 = r"<div>.*\[.*'wrong-1'.*'wrong-2'.*'correct-1'.*'wrong-3'.*'wrong-4'.*\].*</div>"
-        str2 = r"<div>.*\[.*'wrong-3'.*'correct-2'.*'wrong-2'.*'wrong-4'.*\].*</div>"
+        str2 = r"<div>.*\[.*'wrong-3'.*'wrong-1'.*'wrong-2'.*'correct-2'.*\].*</div>"
         str3 = r"<div>\{.*'1_solution_1'.*\}</div>"
         str4 = r"<div>\{.*'1_solution_3'.*\}</div>"
 
@@ -528,8 +522,8 @@ class CapaAnswerPoolTest(unittest.TestCase):
             <problem>
 
             <p>What is the correct answer?</p>
-            <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+            <multiplechoiceresponse answer-pool="4">
+              <choicegroup type="MultipleChoice">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true">correct-1</choice>
@@ -555,5 +549,5 @@ class CapaAnswerPoolTest(unittest.TestCase):
         problem.seed = 723
         the_html = problem.get_html()
 
-        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-3'.*'correct-2'.*'wrong-2'.*'wrong-4'.*\].*</div>")
+        self.assertRegexpMatches(the_html, r"<div>.*\[.*'wrong-3'.*'wrong-1'.*'wrong-2'.*'correct-2'.*\].*</div>")
         self.assertRegexpMatches(the_html, r"<div>\{.*'1_solution_1'.*\}</div>")

common/lib/capa/capa/tests/test_targeted_feedback.py

@@ -142,9 +142,6 @@ class CapaTargetedFeedbackTest(unittest.TestCase):
 
         self.assertRegexpMatches(without_new_lines, r"<div>.*'wrong-1'.*'wrong-2'.*'correct-1'.*'wrong-3'.*</div>")
         self.assertNotRegexpMatches(without_new_lines, r"feedback1|feedback2|feedback3|feedbackC")
-        # Check that calling it multiple times yields the same thing
-        the_html2 = problem.get_html()
-        self.assertEquals(the_html, the_html2)
 
     def test_targeted_feedback_student_answer1(self):
         xml_str = textwrap.dedent("""
@@ -210,9 +207,6 @@ class CapaTargetedFeedbackTest(unittest.TestCase):
 
         self.assertRegexpMatches(without_new_lines, r"<targetedfeedback explanation-id=\"feedback3\">.*3rd WRONG solution")
         self.assertNotRegexpMatches(without_new_lines, r"feedback1|feedback2|feedbackC")
-        # Check that calling it multiple times yields the same thing
-        the_html2 = problem.get_html()
-        self.assertEquals(the_html, the_html2)
 
     def test_targeted_feedback_student_answer2(self):
         xml_str = textwrap.dedent("""
@@ -346,9 +340,6 @@ class CapaTargetedFeedbackTest(unittest.TestCase):
         self.assertRegexpMatches(without_new_lines, r"<targetedfeedback explanation-id=\"feedbackC\".*solution explanation")
         self.assertNotRegexpMatches(without_new_lines, r"<div>\{.*'1_solution_1'.*\}</div>")
         self.assertNotRegexpMatches(without_new_lines, r"feedback2|feedback3")
-        # Check that calling it multiple times yields the same thing
-        the_html2 = problem.get_html()
-        self.assertEquals(the_html, the_html2)
 
     def test_targeted_feedback_no_show_solution_explanation(self):
         xml_str = textwrap.dedent("""