maths / moodle-qtype_stack

Stack question type for Moodle
GNU General Public License v3.0
142 stars 149 forks source link

Feature request: ability to control whether PRT fires for EMPTYANSWER #1227

Closed georgekinnear closed 1 week ago

georgekinnear commented 3 months ago

I'd like to use the allowempty input option to have a question where there are (say) three input boxes but the student can choose how many to complete.

Rather than get into the complicated task I have in mind, let's suppose I want to ask students to find the solutions of x^2+5x+6=0, and give them three input boxes.

I want to have a PRT tied to each of the inputs, that checks that particular answer and gives some feedback (e.g. "no, 4 is not a correct solution")

I also want to have an "overall" PRT that depends on all three inputs and gives some overall evaluative comment (e.g. "there are more solutions!", or "you have repeated a solution")

It seems that I can use allowempty on the three input fields to get the "overall" PRT to work, which is nice. But it means that the three individial PRTs also fire. It would be nice if there was an option within a PRT to say (for each of the inputs that it depends on) whether it should fire if that answer is empty (i.e., EMPTYANSWER).

aharjula commented 3 months ago

So the current logic of PRTs firing only when all the inputs are valid does not work in a sensible way when all the inputs are allowed to be empty and thus valid.

As the allowing of empty is an input side setting, one cannot easily mix behaviours unless the PRT also allows some definitions. The first thing that comes to my mind is to add a trigger setting to PRTs that has these options:

  1. The normal logic of firing when all the referenced inputs are valid.
  2. Same, but at least one must be non-empty, i.e. consider invalid for this PRT if all members of the input set are EMPTYANSWERs.
  3. Don't consider empty as valid for this PRT at all, i.e. consider invalid for this PRT if EMPTYANSWER is present in the input set.

As far as I can now see, that should cover even cases where there are multiple allowempty inputs as the only inputs to PRTs. And in particular the second option should handle the most common issue related to allowempty, i.e. requiring at least one to be filled. Basically, 2&3 should cover the problematic "all empty"-case that may appear in your example when the common PRT requires those input to be allowemptys and the specific PRTs cannot deal with that.

Naturally, this would slightly complicate the PRT logic but on the code side we are talking of about dozen lines and few thousand lines of unit-tests. Basically, this has no effect to the internal PRT logic, it only relates to the PHP logic that checks whether a particular PRT needs to be evaluated, and that logic can easily see the input values and their validity.

In any case up to Chris to decide if we want to complicate PRTs... As it is already possible to eliminate whatever visual effect the PRTs triggering might have, "simply" by identifying the case of EMPTYANSWER and using STACK-JS to hide whatever appears. However, that won't hide the 0 mark, coming from the PRT triggering, from Moodle attempt logs, which might be the more relevant bit here.

sangwinc commented 3 months ago

@georgekinnear thanks for this suggestion. From an educational perspective this makes a lot of sense, especially since we now have formative PRTs, and we support "empty answers" within the overall model.

@aharjula I can see that using JS to hide things might simplify the PHP code a bit (!) but I'd prefer to have a properly supported option within the PHP code to keep consistency.

Having the PRT's auto-detect "valid" inputs considerably simplifies the assessment logic.

@georgekinnear: can you suggest how you would expect to author this?

One option (not yet mentioned) would be to introduce a new option in the input to "allowemptyfor" and then pass the PRT names to the input. It really doesn't matter if specifying the linking input<->PRT is done in the PRT or the input. This has the benefit of not changing the PRTs at all. The only logic which changes is in evaluating whether this input is considered valid for this PRT.

sangwinc commented 3 months ago

Note, there is also now some additional logic in the API. https://github.com/maths/moodle-qtype_stack/blob/master/api/controller/GradingController.php#L72

georgekinnear commented 3 months ago

Thanks for the quick replies about this!

I think rather than the 1/2/3 options @aharjula described, I had been imagining some way in the PRT to specify which of the inputs you require to be nonempty - somewhere around the text that appears saying: "This potential response tree will become active when the student has answered: ans1, ans2, ans3"

I'm not sure what would be a good UI for that. One thought would be to have another line after that saying something like "The following inputs are allowed to be empty: ans2, ans3" (if for instance I've only set allowempty on those two). Then checkboxes (or an input box where you can list the names) to indicate which ones must be nonempty for the PRT to fire.

sangwinc commented 3 months ago

I've reviewed the current logic for dealing with empty answers. Currently, if the "allowempty" option is set in an input and we have a "blank" response then the response is modified at validation time. E.g. here: https://github.com/maths/moodle-qtype_stack/blob/master/stack/input/inputbase.class.php#L1465C17-L1465C38 Note, this modification from blank input to a sensible Maxima expression is done once, before we consider which PRT might depend on this. The result is then a "valid" input (albeit a special value).

The precise nature of this modification depends on the type of input. E.g. we could have EMPTYANSWER, [EMPTYANSWER] or for matrices something which reflects the expected size/shape of the matrix. All this is to simplify type checking in any PRTs which rely on the input.

By design we have no mechanism for PRTs to interfere with empty answer or validity checking. That is, there is no mechanism for a PRT to specify additional rules.

Adding in this feature is going to need a considerable re-engineering of the core logic of STACK and would amount to a substantial project. One of those easy to ask for, but actually surprisingly difficult to implement requests I'm afraid @georgekinnear.

georgekinnear commented 3 months ago

Aha. I'm not quite sure what you mean by "no mechanism for a PRT to specifcy additional rules"?

Might it be possible to have a new box before the "Feedback variables" where the teacher can provice an optional bit of code to control the firing of the PRT? Perhaps called something like "Stop firing the PRT if the following returns TRUE", where the teacher could write something like is(ans1 = EMPTYANSWER).

aharjula commented 3 months ago

@sangwinc That JS comment was about authors already having the tools to hide stuff, whatever solution we might have would naturally not act at the JS level.

@georgekinnear The 1,2,3 option set is about as far as I would go if I were the one extending STACK, as it keeps the finer selection logic out of the hands of the author. If we had logic where one could give arbitrary expressions for triggering PRTs, it might lead authors towards some fairly complicated PRT forest constructs, and the next logical thing they would then ask would be the ability to define the PRT point weight based on logic inside the PRT (someone would state that they now have mutually exclusive (this comes from the control of triggering, even that 1,2,3 would in some sense give this) overlapping PRTs that should be able to give points and the total value of the question should be able to match that). And if we were to give that option for controlling triggering at that level it could then difficult to not also give the tools to adjust weights, and at that point understanding the grade construction in STACK question would become somewhat more complicated issue and managing some very basic constraints coming from basic assumptions related to "fair grading" might become ever so slightly difficult.

I, for one, like the simplicity of the PRT model as it is currently designed and would not want those complications. The balance between the accessible level of complexity and the capabilities available in STACK is, from my point of view, at the correct place, and the capabilities this might bring don't really match the added complexity in my mind, in particular, if we were to allow arbitrary logic.

sangwinc commented 3 months ago

@georgekinnear I hadn't thought of stopping a PRT with a user-defined function. That would be relatively simple to implement, and would work within the existing model. I share @aharjula's well-founded concerns about blurring and making the model more complex especially in sorting out scores. However, I can see situations where empty/non-empty situations combined with formative PRTs might be very useful from an educational perspective.

So, by default we have a function

stack_prt_stopp(ex):=false;

or a constant

stack_prt_stopp:false;

which is a predicate to stop the execution of the PRT. If you choose to allow empty answers in some inputs, then we could halt some PRTs if that predicate returns true. The outcome of that would be the same as if the PRT had simply not fired at all (blank/invalid inputs).

Would that give you what you need? Are you sure you don't want to mess around with scores etc.?!

sangwinc commented 3 months ago

@aharjula I think we would have to add some logic here https://github.com/maths/moodle-qtype_stack/blob/c8ea383cd039f2b5bf5c0ce37dcdb8a8fc5f09c8/question.php#L1268 to check if the PRT evaluation is to be disregarded, after it has been evaluated. Is there anywhere else to add in that logic?

aharjula commented 3 months ago

Well, if you really want that, then you would compile the logic into the PRT in the stack/prt.class.php making it return something different if this is the case, and then make it so that you could easily identify this happening from the unpacked result in stack/prt.evaluatable.class.php, which you would then ask for this in that place you reference.

But do note that I do expect that there will be authors that will write a question with three allow empty inputs and the following four PRTs and will ask for them to be able to give scores in a sensible way so that the last one will be able to give full points for the question.

/* For the first pair. */
stack_prt_stopp(ex):= not is(ans1=EMPTYANSWER) and not is(ans2=EMPTYANSWER) and is(ans3=EMPTYANSWER);
/*...*/
stack_prt_stopp(ex):= not is(ans1=EMPTYANSWER) and is(ans2=EMPTYANSWER) and not is(ans3=EMPTYANSWER);
stack_prt_stopp(ex):= is(ans1=EMPTYANSWER) and not is(ans2=EMPTYANSWER) and not is(ans3=EMPTYANSWER);
/* When they are all present... */
stack_prt_stopp(ex):= not is(ans1=EMPTYANSWER) and not is(ans2=EMPTYANSWER) and not is(ans3=EMPTYANSWER);
georgekinnear commented 3 months ago

Thanks, I think the predicate approach would give me what I need.

I'm thinking about this in the context of a formative question, so I'd not given much thought to marks. I see @aharjula's point about it potentially getting complicated (e.g. if the student can give a fully correct answer in different ways, using just ans1 and ans2, or using all three - then you might want prt1 tied to ans1 to count for 1/2 or 1/3 of the question) but I think for teachers wanting to go down that road, they could just have a complicated prt_overall that relies on all the inputs and gives an overall score.

By the way, does this mean the earlier suggestion about some checkbox-based UI on the teacher interface might work after all? Essentially it would map on to writing a stack_prt_stopp function, but it would give a much more constrained set of options to the teacher (i.e. they can't write an arbitrary function, but only specify one using the checkboxes).

georgekinnear commented 3 months ago

Also - perhaps the predicate approach is relevant to the discussion about "postprocessing logic" in https://github.com/maths/moodle-qtype_stack/issues/870?

As I understand it, the idea there is to have a block of code that runs before all the PRTs - so the predicate functions could maybe live in that block?

aharjula commented 3 months ago

Checkboxes would be an option for less arbitrary logic, but I would want two sets of checkboxes so that they could provide my suggested 1, 2, and 3 options with some extra precision. Basically, I would:

  1. Keep the current logic of the PRT only referencing inputs referenced in the PRT code, i.e., no need to draw the full set of checkboxen for those other inputs that are not referenced.
  2. If and only if those referenced inputs happen to have some allowblanks, then we need to start asking some questions in the form of checkboxes, but only about the ones that have allowblank.
    • If there is only one input and it is an allowblank then we are in a problem as we are essentially not asking a question we will be forcing the checking of a box as that is the only sane answer.
    • If there are more inputs and they are all allowblanks then we need to ask which ones of them need to be non-blank for this to be a valid input or alternatively which subset of them needs to include at least one non-blank case. These will require those two different sets of checkboxes.
    • Might as well ask those questions also when we have some non allow-blanks in the picture. The second set only needs definition when there are more than one allowblank referenced by the PRT.

Note that this would not allow the definition of arbitrary logic like "trigger when exactly one of {a,b,c} and at least one of {d,e,f} has a value", but should cover the more common case of "at least one of {...}" which the thing that I would like to see (if we go here then I would want this, otherwise not a priority) to fix the problem that we have with PRTs + "fully allowblank referenced inputs set".

Like my 1,2,3 suggestion, the checkbox option would be perfectly evalautable within PHP, and would be simple and safe (and not yet another place where the author can create code that could break in interesting ways). Basically, only modify the conveniently named functions has_necessary_prt_inputs to reference whatever new fields there are in the model describing those checkboxes.

As to #870, that is something different. It would focus on adding logic on the input level. If I understand your comment correctly, you are thinking #763, which is a problem as it mixes inputs and PRTs in an inconvenient way. I am not aware of any plans for a common code block that would exist between the inputs and PRTs and act as a nexus for routing stuff between them, but I am very aware of all sorts of reasons why I would not build such a thing.

If I understood Chris correctly, then stack_prt_stopp would not actually be a predicate as it would happen inside the PRT and would essentially trigger early exit or abort from that PRT. It would be defined inside the PRT to help with the detection of inputs required for evaluation of the PRT and its "predicate", and if it were to exist in some common code elsewhere, then it would need to be targetted to that PRT in some way, after which tracing its input requirements back would be a problem.

sangwinc commented 3 months ago

Thinking further on %stack_prt_stopp, this is a constant. Initially it is defined as false.

  1. If %stack_prt_stopp is true (in the feedback variables) then we don't execute the PRT at all - we bail at the end of the feedback variables. This can then act as a guard clause for the PRT code.
  2. If %stack_prt_stopp is true after the PRT has executed, then ignore the the results of the PRT as if "nothing happened". I need to think further on this, but it means a branch in the PRT can (perhaps) indicate we should ignore the outcome.
aharjula commented 3 months ago

For the second case, we need to check if you could use feedback and the [[define/]] block for that, I don't remember if it allows the definition of identifiers starting with %, but surely it could be made to do so as it already has some special behaviour like the ability to define the same identifier multiple times. If going through CASText then the CASText compiler will also need to know about this so that it does not protect the variable as an internal to CASText thing and allows the changed value to leak out.

So to abort a PRT from being considered for anything would happen by A. setting %stack_prt_stopp:true in the feedback variables or B. including [[define %stack_prt_stopp="true"/]] in the feedback of a branch that needs to stop this.

sangwinc commented 3 months ago

OK @aharjula, since this really is a special case and an edge case I think I'll limit myself to feedback variables only for the initial fix. We can wait for a really compelling case for anything more complex to deal with....

georgekinnear commented 3 months ago

Thanks, this sounds like it will work nicely.

sangwinc commented 3 months ago

@georgekinnear could you check the docs in the above commit please, and then also complete the docs with an example here https://github.com/maths/moodle-qtype_stack/commit/a5b498685f81fd497a9da827ed30f97f9e6db1d0#diff-54d76f198c5263fc9920dcf4affbf879fc632b81ca37d8b2a7d56e79694799e1R138 and write an example question using this feature we can add as a test case?

Ideally, you'd complete the stub test case here please! https://github.com/maths/moodle-qtype_stack/commit/a5b498685f81fd497a9da827ed30f97f9e6db1d0#diff-cb51c63dd6df37f0269dcf20e8afa7615695db0bba910c38c728196803766e35R4192

That way we can use "test driven design". Once that's done I'll add the feature. Nice suggestion, thank you.

sangwinc commented 1 week ago

@georgekinnear I'm creating a list of issues to include in the next release for use from Jan 2025. Please can you provide an example of why you asked for this feature? We can add that to the docs, and as a test case.

sangwinc commented 1 week ago

Thanks for the example, I've added that to the docs. This is a potentially very useful feature, I appreciate you suggesting and explaining it!