-
Notifications
You must be signed in to change notification settings - Fork 4.3k
jeff/feature-answer-pool #1828
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
jeff/feature-answer-pool #1828
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,8 @@ | |
|
|
||
| from pytz import UTC | ||
|
|
||
| from random import Random | ||
|
|
||
| # dict of tagname, Response Class -- this should come from auto-registering | ||
| response_tag_dict = dict([(x.response_tag, x) for x in responsetypes.__all__]) | ||
|
|
||
|
|
@@ -381,11 +383,128 @@ def get_answer_ids(self): | |
| answer_ids.append(results.keys()) | ||
| return answer_ids | ||
|
|
||
| def sample_from_answer_pool(self, choices, rnd, num_choices): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this code should live in MultipleChoiceResponse in responsetypes.py Perhaps instead of doing this processing right before rendering, we instead do it right after the choice ids are assigned. |
||
| """ | ||
| Takes in: | ||
| 1. list of choices | ||
| 2. random number generator | ||
| 3. max number of total choices to return | ||
|
|
||
| Returns a list with 2 items: | ||
| 1. the solution_id corresponding with the chosen correct answer | ||
| 2. (subset) list of choice nodes with 3 incorrect and 1 correct | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think you mean N here |
||
| """ | ||
|
|
||
| correct_choices = [] | ||
| incorrect_choices = [] | ||
| subset_choices = [] | ||
|
|
||
| for choice in choices: | ||
| if choice.get('correct') == 'true': | ||
| correct_choices.append(choice) | ||
| elif choice.get('correct') == 'false': | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is "correct" always present here, or just for true?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I'm nervous about values other than the exact strings "true" and "false", perhaps we want something more like: try:
is_correct = lower(choice.get('correct', 'false')) == 'true'
except TypeError:
# log a warning
is_correct = False
if is_correct:
correct_choices.append(choice)
else:
incorrect_choices.append(choice)It is not clear in the code as written what happens in the following cases:
There may already be a robust boolean parsing function for edxml somewhere else, @cpennington - do you know of anything like this? |
||
| incorrect_choices.append(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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does num_choices relate to len(choices)? I'm assuming they are equal, but since both are passed into this function it is a bit unclear... This may be more clearly expressed as: if len(choices) < 1 or len(correct_choices) < 1:
# handle invalid input
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I apologize, I now understand the intent of num_choices. So I guess this could be: if len(choices) == 0 or len(choices) < num_choices:
# raise an error related to not enough choices
if len(correct_choices) < 1:
# raise an error related to not having enough correct choices |
||
| return [] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a personal preference but I tend to prefer raising an exception on invalid input like this, instead of returning an empty result, since I think it can provide a lot more detailed information about what went wrong. Given a stack trace, the code and the input, it becomes a lot easier to figure out exactly what's wrong. |
||
|
|
||
| # 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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a bit confusing to me. Is it valid for num_incorrect != len(incorrect_choices)? |
||
|
|
||
| # 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] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could simplify this using "choice": correct_choice = rnd.choice(correct_choices) |
||
| subset_choices.append(correct_choice) | ||
| solution_id = correct_choice.get('explanation-id') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would note in the docstring that it's expected and OK for this to be |
||
|
|
||
| # 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could replace this loop by using sample: selected_incorrect_choices = rnd.sample(incorrect_choices, num_incorrect)
subset_choices.extend(selected_incorrect_choices) |
||
|
|
||
| # 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use random.shuffle here: rnd.shuffle(subset_choices)I would also note in the docstring that this function returns the subset in random order.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also consolidate this with @nparlante 's obfuscation / shuffle approach. I'm thinking we might be able to get away with having only one location in the code where we shuffle the choices. |
||
|
|
||
| return [solution_id, subset_choices] | ||
|
|
||
| def tree_using_answer_pool(self, 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 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 | ||
|
|
||
| These problems are colloquially known as "Gradiance" problems. | ||
| """ | ||
|
|
||
| query = '//multiplechoiceresponse[@answer-pool]' | ||
|
|
||
| # There are no questions with an answer pool | ||
| if not tree.xpath(query): | ||
| return | ||
|
|
||
| # 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 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These validations should provide some user feedback instead of silently passing over the input. Perhaps raise an exception? Or log something? |
||
|
|
||
| # Grab the first choicegroup (there should only be one within each <multiplechoiceresponse> tag) | ||
| choicegroup = mult_choice_response.xpath('./choicegroup[@type="MultipleChoice"]')[0] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer this raises a more descriptive error than a "IndexError" if no valid choicegroups are found. |
||
| 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) | ||
|
|
||
| # Add back in randomly selected choices | ||
| for choice in subset_choices: | ||
| choicegroup.append(choice) | ||
|
|
||
| # 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 = mult_choice_response.xpath('./following-sibling::solutionset') | ||
| if len(solutionset) != 0: | ||
| solutionset = solutionset[0] | ||
| solutions = solutionset.xpath('./solution') | ||
| for solution in solutions: | ||
| if solution.get('explanation-id') != solution_id: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checking the assumption here, but it looks like it's OK to have multiple solutions with the same What should happen if the following input is provided? <solutionset>
<solution>Foo</solution>
<solution>Bar</solution>
</solutionset> |
||
| solutionset.remove(solution) | ||
|
|
||
| return | ||
|
|
||
| def get_html(self): | ||
| ''' | ||
| Main method called externally to get the HTML to be rendered for this capa Problem. | ||
| ''' | ||
|
|
||
| self.tree_using_answer_pool(self.tree) | ||
| html = contextualize_text(etree.tostring(self._extract_html(self.tree)), self.context) | ||
|
|
||
| return html | ||
|
|
||
| def handle_input_ajax(self, data): | ||
|
|
@@ -562,8 +681,7 @@ def _extract_html(self, problemtree): # private | |
| # other than to examine .tag to see if it's a string. :( | ||
| return | ||
|
|
||
| if (problemtree.tag == 'script' and problemtree.get('type') | ||
| and 'javascript' in problemtree.get('type')): | ||
| if (problemtree.tag == 'script' and problemtree.get('type') and 'javascript' in problemtree.get('type')): | ||
| # leave javascript intact. | ||
| return deepcopy(problemtree) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is too long for a changelog entry - maybe a better idea would be to provide a brief description (first three lines would suffice), and then a link to some documentation on the edx-platform wiki or some other public place.