maths / moodle-qtype_stack

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

Some discussion about best practice with bespoke validators and feedback #1124

Open LukeLongworth opened 8 months ago

LukeLongworth commented 8 months ago

I'm really loving the new(ish) custom validation options that STACK 4.4.3 introduced and am finding many places that they are useful. They are making a bigger impact on how I approach question writing than any other new feature I can remember, which surprises me! I'm making this post because I find myself disregarding a couple of comments from the documentation and I was interested to hear what other users/developers think.

Mainly, the docs emphasise that you cannot overwrite existing validation. This makes sense to me, but the allure of custom validation messages has me doing my best to avoid using other existing features.

For example, I have a bank of differential equations questions, some of which use x as the independent variable and others of which use t. Previously I might have used the "forbidden variables" feature to prevent students from using the wrong variable, or perhaps the checkvars option. Nowadays I avoid this, as the default error messages are always (naturally) context-independent. I want to tell the students explicitly "You can't use t here because the independent variable is x.", which is very easy to do with validate_contains_t(ex):= if member(t,listofvars(ex)) then return("You can't use t here because the independent variable is x.") else return("").

Another small example is when working with differentials. Students sometimes type in d*x instead of the desired dx, and I would prefer to tell them "You should use dx instead of d*x in your answer" rather than just telling them that d is forbidden.

Similarly, I've taken to using custom validation functions in lots of places that Insert Stars is intended to be used. I've been combining the all_one_letter_variables and nofunctions validators to notify students that they probably meant to include an . This has had me wanting to edit the nofunctions error message to say "perhaps you meant x(y+1) rather than x(y+1)" or something to that effect, because students may not understand the default error message if they didn't mean to type x as a function. Besides importing the function I don't have any control over it.

I find myself stuck between a few ideas; I don't feel like I can meaningfully contribute these to the code base because they are so context dependent, but I do find myself copying them between a large number of questions. I want to be using the basic, core functions so that I can rely on their continued support, but it's usually more useful to me to create my own workarounds.

Would it be possible to give question creators more access to those default error messages? Something like an optional argument somewhere that defaults to the existing message but allows for a custom message to overwrite it? Another idea could be to include a "validator constructor" somehow. I could see myself running a function like validate_no_var_constructor(t, msg) that constructs a validator called validate_no_var that checks whether t is present and gives the custom error message msg if it is. This is something I would be happy to work on and contribute if it sounds feasible and useful. Perhaps my existing strategy is the best one already!

I'm mostly curious to hear others' thoughts on this one.

aharjula commented 8 months ago

Good to hear that this feature is proving to be as useful as I thought it would be.

As to the documentation, it would indeed be nice if we could convey more clearly that you cannot overwrite certain non-optional core validation, but all validation that is optional can naturally be turned off and a replacement given through this system. Also, unfortunately, you cannot tune the implementations of the optional core validation logic at this time.

Many things are possible, and what you suggest in the form of parametric validators that can be generated with a function is indeed something that might be worth exploring in the validator set. Basically, building validators this way would make sense even if we are not tuning the messages. It just happens to be somewhat difficult to fix the parameter values for the lambdas, i.e. this won't work as A,B won't have the values one might expect:

validator_for_A_instead_B(A,B):=
 lambda([ex],
  if member(B,listofvars(ex)) then 
    sconcat("You can't use ",B," here use ",A," instead.") 
   else true);

To work around that, one would need to construct the lambda from parts with evaluation (not just simplification) disabled and deal with some rather complicated things, some of which would require some extra Maxima packages. It would be interesting to see if someone figures out a simple way to do that and would generate a sample. I, for one, gave up once I figured out how complex it is without certain features. Basically, this makes it somewhat unexpectedly hard to push in even a message through any validator-constructor function. Basically, the message would need to be outside the validator and it would need to be accessible from a static location, and handling that is difficult and leads to the problem that you cannot generate multiple similar validators with different messages for the same question.

But if we are only talking about messages in general (and do not need to have validator instance-specific messages), then there is hope on the horizon. One of my longer-term projects has been to make it so that every single bit of STACK Maxima side strings would be an inline-CASText string instead of the current somewhat less readable strings where we have slightly less obvious positional arguments: https://github.com/maths/moodle-qtype_stack/blob/b4136712b00dc3940d6be10101284cd657fd0406/stack/maxima/stackmaxima.mac#L2997 we would write something like this:

...StackAddFeedback("", castext("[[commonstring key='ATInt_generic' expected='SBdisp' var='var' got='SAd' /]]")

Now, that might not seem like any sort of solution immediately, but it would allow us to both have language-pack strings for most usages, even in those contrib-validators and if we want, we could also use the [[template]]-block for all of those strings we want to make question level overridable (so probably all strings).

So if things go in the direction I am thinking, then in the future, we might have contrib-validators declared in this style:

validate_nofunctions(ex):= block([op1],
  if atom(ex) then return(""),
  op1:ev(op(ex)),
  op1:apply(properties, [op1]),
  if ev(emptyp(op1) or is(op1=[noun]),simp) then return(castext("[[template mode='default' name='validate_nofunctions_warning']][[commonstring key='validate_nofunctions_warning'/]][[/template]]")),
  apply(sconcat, map(validate_nofunctions, args(ex)))
);

As that is a bit long and repetitive, there would probably be a new block for this task handling both the template and commonstring bits.

It would then have the language pack string of validate_nofunctions_warning => "User-defined functions are not permitted in this input." but it would also have a template that would probably have the same identifier that could be overridden at the question level simply by placing something like this in the question variables:

dummy:castext("[[template name='validate_nofunctions_warning']]Thou shall not use functions here![[/template]]");
/* When that block evaluates the template will have a new implementation.
   And it basically evaluates there when it gets assigned to that variable. */

Do note that none of this is possible (for the STACK-Maxima libraries) at this moment as it requires one massive overhaul of our Maxima side code, but the tooling required starts to near completion, so this might become an option even for inner-logic. You could, however, already use this method with includes and question-side logic.

In any case, we should probably look into figuring out how we could actually have parametric validator constructors and thus validator instance-specific messages, so that is a new development direction, even if some of the issues could be solved with inline-castext+[[templates]].

sangwinc commented 8 months ago

Thanks @aharjula, we're typing in parallel on a Friday morning. I've responded to some of the other ideas in your post, and I'm running unit tests on some things which are possible. This includes better feedback within the existing validator code.

@LukeLongworth, thanks for this very thoughtful post.

I agree - the bespoke validation is a case where I've genuinely made is easier for other people to contribute. This is going to be a fine balance between letting everyone do their own thing, and having centrally supported features. PLEASE, if you create a useful bespoke validator please contribute it back here:

https://github.com/maths/moodle-qtype_stack/tree/master/stack/maxima/contrib

I've already added a couple in this file. https://github.com/maths/moodle-qtype_stack/blob/master/stack/maxima/contrib/validators.mac which many people have asked for.

  1. "the docs emphasise that you cannot overwrite existing validation"

Yes, there are some very important parts of the validation system which must always remain in place. However, we might decide to make it easier/more flexible to fine tune in the future, as Matti has mentioned.

  1. "I do find myself copying them between a large number of questions."

The idea of stack_include was to avoid this. You can have private libraries which you use. This, of course, makes your questions very fragile to the continuing availability of your library. Proposals for centrally supported functions are really welcome!

  1. "Would it be possible to give question creators more access to those default error messages?"

Interesting suggestion. The language pack in English is here: https://github.com/maths/moodle-qtype_stack/blob/master/lang/en/qtype_stack.php

One problem, particularly with the insert * logic, is that the error messages are created by PHP, e.g.

To keep the design coherent, we have "filters" which are all separated out and are here https://github.com/maths/moodle-qtype_stack/tree/master/stack/cas/parsingrules

This is a good example: https://github.com/maths/moodle-qtype_stack/blob/master/stack/cas/parsingrules/022_trig_replace_synonyms.filter.php

We could have designed STACK to make all these replacements in Maxima, with an "alias" command. But we chose to do it in PHP instead.

On the other hand, question variables and the custom validation is part of the Maxima code. We have to assume that by the time the student's (invalid!) answer gets as far as Maxima we can at least parse their expression into a maxima object. If parsing fails, then we can't possibly apply the custom validator.

There is currently no mechanism in the question to override a language string. That, itself, is an interesting feature request. It's perfectly reasonable actually. Affecting language strings globally within a question might not be too hard, but that's not the whole story, of course. People will want to change the language strings locally, e.g. within an input or within a PRT (node!). I'm not sure how that would be authored.

aharjula commented 8 months ago

Chris, I have to say that I would oppose any feature that would allow overriding language strings with any logic that would change the string in the Moodle runtime dictionary. It would immediately break things as then neighbouring questions would suffer from each other's overrides. This is the reason why I would prefer [[template]] to be used, as it is something that would be truly question-specific and even more finely targettable within the question as it goes through that question's CAS session. Language strings can and should still be in the default template, but if one wishes to override them then one overrides the template and not the language string.

sangwinc commented 8 months ago

Agreed: nothing that would allow overriding language strings with any logic that would change the string in the Moodle runtime dictionary! It needs to be question specific. A block is an interesting suggestion, thanks!