From ada9ff7f27925b21da41e26873b39b8f04a61ed0 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 31 Jan 2013 10:45:11 -0500 Subject: [PATCH 1/4] Fix randomization bug in capa. (Note: capa_problem was still doing randomization internally, but now it does what's actually intended) --- common/lib/xmodule/xmodule/capa_module.py | 33 ++++++++++++++++------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index f33da6e3a4..27bf0c4cb1 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -2,6 +2,7 @@ import cgi import datetime import dateutil import dateutil.parser +import hashlib import json import logging import traceback @@ -25,6 +26,22 @@ log = logging.getLogger("mitx.courseware") #----------------------------------------------------------------------------- TIMEDELTA_REGEX = re.compile(r'^((?P\d+?) day(?:s?))?(\s)?((?P\d+?) hour(?:s?))?(\s)?((?P\d+?) minute(?:s)?)?(\s)?((?P\d+?) second(?:s)?)?$') +# Generated this many different variants of problems with rerandomize=per_student +NUM_RANDOMIZATION_BINS = 20 + +def randomization_bin(seed, problem_id): + """ + Pick a randomization bin for the problem given the user's seed and a problem id. + + We do this because we only want e.g. 20 randomizations of a problem to make analytics + interesting. To avoid having sets of students that always get the same problems, + we'll combine the system's per-student seed with the problem id in picking the bin. + """ + h = hashlib.sha1() + h.update(str(seed)) + h.update(str(problem_id)) + # get the first few digits of the hash, convert to an int, then mod. + return int(h.hexdigest()[:7], 16) % NUM_RANDOMIZATION_BINS def only_one(lst, default="", process=lambda x: x): """ @@ -138,13 +155,9 @@ class CapaModule(XModule): if self.rerandomize == 'never': self.seed = 1 - elif self.rerandomize == "per_student" and hasattr(self.system, 'id'): - # TODO: This line is badly broken: - # (1) We're passing student ID to xmodule. - # (2) There aren't bins of students. -- we only want 10 or 20 randomizations, and want to assign students - # to these bins, and may not want cohorts. So e.g. hash(your-id, problem_id) % num_bins. - # - analytics really needs small number of bins. - self.seed = system.id + elif self.rerandomize == "per_student" and hasattr(self.system, 'seed'): + # see comment on randomization_bin + self.seed = randomization_bin(system.seed, self.location.url) else: self.seed = None @@ -669,18 +682,18 @@ class CapaDescriptor(RawDescriptor): # TODO (vshnayder): do problems have any other metadata? Do they # actually use type and points? metadata_attributes = RawDescriptor.metadata_attributes + ('type', 'points') - + def get_context(self): _context = RawDescriptor.get_context(self) _context.update({'markdown': self.metadata.get('markdown', '')}) return _context - + @property def editable_metadata_fields(self): """Remove metadata from the editable fields since it has its own editor""" subset = super(CapaDescriptor,self).editable_metadata_fields if 'markdown' in subset: - subset.remove('markdown') + subset.remove('markdown') return subset From e431378f46a812afc5a562cedd87babe7beb5bec Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 31 Jan 2013 12:14:01 -0500 Subject: [PATCH 2/4] Add first pass at a randomize module --- common/lib/xmodule/setup.py | 1 + .../lib/xmodule/xmodule/randomize_module.py | 122 ++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 common/lib/xmodule/xmodule/randomize_module.py diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 29227c3188..446078ffcf 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -28,6 +28,7 @@ setup( "error = xmodule.error_module:ErrorDescriptor", "problem = xmodule.capa_module:CapaDescriptor", "problemset = xmodule.seq_module:SequenceDescriptor", + "randomize = xmodule.randomize_module:RandomizeDescriptor", "section = xmodule.backcompat_module:SemanticSectionDescriptor", "sequential = xmodule.seq_module:SequenceDescriptor", "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", diff --git a/common/lib/xmodule/xmodule/randomize_module.py b/common/lib/xmodule/xmodule/randomize_module.py new file mode 100644 index 0000000000..0bc26c21bf --- /dev/null +++ b/common/lib/xmodule/xmodule/randomize_module.py @@ -0,0 +1,122 @@ +import json +import logging +import random + +from xmodule.mako_module import MakoModuleDescriptor +from xmodule.x_module import XModule +from xmodule.xml_module import XmlDescriptor +from xmodule.modulestore import Location +from xmodule.seq_module import SequenceDescriptor + +from pkg_resources import resource_string + +log = logging.getLogger('mitx.' + __name__) + +class RandomizeModule(XModule): + """ + Chooses a random child module. Chooses the same one every time for each student. + + Example: + + + + + + + User notes: + + - If you're randomizing amongst graded modules, each of them MUST be worth the same + number of points. Otherwise, the earth will be overrun by monsters from the + deeps. You have been warned. + + Technical notes: + - There is more dark magic in this code than I'd like. The whole varying-children + + grading interaction is a tangle between super and subclasses of descriptors and + modules. +""" + + def __init__(self, system, location, definition, descriptor, + instance_state=None, shared_state=None, **kwargs): + XModule.__init__(self, system, location, definition, descriptor, + instance_state, shared_state, **kwargs) + + # NOTE: calling self.get_children() creates a circular reference-- + # it calls get_child_descriptors() internally, but that doesn't work until + # we've picked a choice + num_choices = len(self.descriptor.get_children()) + + self.choice = None + if instance_state is not None: + state = json.loads(instance_state) + self.choice = state.get('choice', None) + if self.choice > num_choices: + # Oops. Children changed. Reset. + self.choice = None + + if self.choice is None: + # choose one based on the system seed, or randomly if that's not available + if num_choices > 0: + if system.seed is not None: + self.choice = system.seed % num_choices + else: + self.choice = random.randrange(0, num_choices) + + log.debug("********* self.choice = %s", self.choice) + if self.choice is not None: + self.child_descriptor = self.descriptor.get_children()[self.choice] + # Now get_children() should return a list with one element + log.debug("children of randomize module (should be only 1): %s", + self.get_children()) + self.child = self.get_children()[0] + else: + self.child_descriptor = None + self.child = None + + + def get_instance_state(self): + return json.dumps({'choice': self.choice}) + + + def get_child_descriptors(self): + """ + For grading--return just the chosen child. + """ + if self.child_descriptor is None: + return [] + + return [self.child_descriptor] + + + def get_html(self): + if self.child is None: + # raise error instead? In fact, could complain on descriptor load... + return "
Nothing to randomize between
" + + return self.child.get_html() + + def get_icon_class(self): + return self.child.get_icon_class() if self.child else 'other' + + +class RandomizeDescriptor(SequenceDescriptor): + # the editing interface can be the same as for sequences -- just a container + module_class = RandomizeModule + + filename_extension = "xml" + + stores_state = True + + def definition_to_xml(self, resource_fs): + xml_object = etree.Element('randomize') + for child in self.get_children(): + xml_object.append( + etree.fromstring(child.export_to_xml(resource_fs))) + return xml_object + + def has_dynamic_children(self): + """ + Grading needs to know that only one of the children is actually "real". This + makes it use module.get_child_descriptors(). + """ + return True + From e0fb906c0692da41253ac1fe9411c6703f981f44 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 31 Jan 2013 12:14:20 -0500 Subject: [PATCH 3/4] add note about potential bug in verticals. No time to investigate at the moment... --- common/lib/xmodule/xmodule/vertical_module.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/lib/xmodule/xmodule/vertical_module.py b/common/lib/xmodule/xmodule/vertical_module.py index 397bd3e136..14105b41d0 100644 --- a/common/lib/xmodule/xmodule/vertical_module.py +++ b/common/lib/xmodule/xmodule/vertical_module.py @@ -48,3 +48,5 @@ class VerticalDescriptor(SequenceDescriptor): js = {'coffee': [resource_string(__name__, 'js/src/vertical/edit.coffee')]} js_module_name = "VerticalDescriptor" + # TODO (victor): Does this need its own definition_to_xml method? Otherwise it looks + # like verticals will get exported as sequentials... From b6614f693947920dc192c3c9123fa45dde584f44 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Mon, 4 Feb 2013 18:28:22 -0500 Subject: [PATCH 4/4] add initial test that only tests the descriptor --- .../lib/xmodule/xmodule/randomize_module.py | 1 - .../xmodule/tests/test_randomize_module.py | 55 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 common/lib/xmodule/xmodule/tests/test_randomize_module.py diff --git a/common/lib/xmodule/xmodule/randomize_module.py b/common/lib/xmodule/xmodule/randomize_module.py index 0bc26c21bf..f88cdc5efb 100644 --- a/common/lib/xmodule/xmodule/randomize_module.py +++ b/common/lib/xmodule/xmodule/randomize_module.py @@ -61,7 +61,6 @@ class RandomizeModule(XModule): else: self.choice = random.randrange(0, num_choices) - log.debug("********* self.choice = %s", self.choice) if self.choice is not None: self.child_descriptor = self.descriptor.get_children()[self.choice] # Now get_children() should return a list with one element diff --git a/common/lib/xmodule/xmodule/tests/test_randomize_module.py b/common/lib/xmodule/xmodule/tests/test_randomize_module.py new file mode 100644 index 0000000000..6353245f1a --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_randomize_module.py @@ -0,0 +1,55 @@ +import unittest +from time import strptime +from fs.memoryfs import MemoryFS + +from mock import Mock, patch + +from xmodule.modulestore.xml import ImportSystem, XMLModuleStore + + +ORG = 'test_org' +COURSE = 'test_course' + +START = '2013-01-01T01:00:00' + + +from test_course_module import DummySystem as DummyImportSystem +from . import test_system + + +class RandomizeModuleTestCase(unittest.TestCase): + """Make sure the randomize module works""" + @staticmethod + def get_dummy_course(start): + """Get a dummy course""" + + system = DummyImportSystem(load_error_modules=True) + + def to_attrb(n, v): + return '' if v is None else '{0}="{1}"'.format(n, v).lower() + + start_xml = ''' + + + + Two houses, ... + Three houses, ... + + + + '''.format(org=ORG, course=COURSE, start=start) + + return system.process_xml(start_xml) + + def test_import(self): + """ + Just make sure descriptor loads without error + """ + descriptor = self.get_dummy_course(START) + + # TODO: add tests that create a module and check. Passing state is a good way to + # check that child access works... +