answerHints - Fix for the first issue reported in #1170

[taniwallach]

This is meant to fix the first issue reported in #1170

I put back the test that either the score is less than 1 or that checkCorrect was set before triggering a hint when "wrong" is not a subroutine.

The intention is to fix the backwards compatibility for existing content. The "price" is that any very new problems which did not set checkCorrect and expect the answerHint code to process an non-code option will need to add it in explictly.

Note: The older version of this test had a third "option" (another '||'). Based on the discussion in #964 (comment) the || AnswerHints::Compare($correct, $wrong, $ans) portion of the old test was removed. @dpvc noted there that it was missing a negation, but felt it could be removed anyway. He removed even more - but the removal of the entire conditional about score < 1 is what is causing my problem.

[drgrice1]

This looks good. I didn't notice that so much was removed in #974 on this. It doesn't seem that this should have been removed.

[dpvc]

I would like to look into this further, but haven't had the chance. Please wait before merging. Thanks!

[dpvc]

I think the AnswerHints code is correct as it stands, and I would object to this change. My reasoning for removing the conditions in #974 was that if an answer is given explicitly, it should be checked, regardless of whether it or the student answer is correct. If it is explicitly in the AnswerHints list, then if it is the correct answer, its message is clearly meant to be used, and you shouldn't have to add another flag to get it. The checkTypes and checkCorrect flag were really designed for use with a CODE reference, not with explicit answers (the removed code in #974 was completely broken, as discussed in that PR). Any explicit answer should be processed, except when there is already a message in place and the replaceMessage flag is not set. The reason for that exception is that it stops the later messages from being used if an earlier one has been issued. (It is possible to handle that in other ways, so that AnswerHints messages take precedence over other messages already in place, but I'm not sure it is a good idea. I wouldn't want to lose syntax errors or other such messages, for example.)

As you have noticed, the checker function as written will never work with AnswerHints() regardless of this change, for one simple reason: it doesn't take the $correct answer into account. Why that matters is that AnswerHints uses that to check the answers in your list of test answers with messages. That is, AnswerHints calls the correct answer's cmp_equal() method using an adjusted copy of $ans using the test answer as the correct answer (leaving the students answer as itself), and cmp_equal() uses the checker if there is one. So an answer checker that doesn't check the $correct answer against the $student answer will never work properly with AnswerHints, as it will never be able to detect the answers in your list.

In your case, if the student answer is correct, then every answer in the AnswerHints list will be considered correct, and if the student answer is false, then no answer in the AnswerHints list will match it. That is an issue with the checker code, not the AnswerHints code. I was going to recommend a change like the one you have already suggested:

return $correct == $student unless $correct == $ans6c;

but your test for the answer filter name is probably better. Most checker functions don't need to do this, as they use the $correct answer as part of their test, but because this one is unusual in not doing so, it must take extra steps to handle the AnswerHints checks.

Note, however, that this still is not enough to make your AnswerHints list work properly, as the message for the last two answers (sin(x) and exp(x)) will never be issued. That is due to the fact that when the student answer is checked and is incorrect (as it would be if it were either of those two answers), the checker sets its own answer message, and so that message is not replaced. You would need to add replaceMessage options to those two AnswerHints entries in order to get them to work. The DNE entry only "works" due to a fluke, which is that when the student answer is DNE, the attempt to take its derivative fails, the checker crashes at that point, and the answer message is not set, so the AnswerHints entries are checked after all, and the first one produces the DNE message (provided you include the line above in the checker). The change in this PR is not needed for that -- it is the line above that resolves the problem.

Also note that the checkTypes option is not needed for the DNE entry, as it only applies to CODE answers, and has no effect on explicit answers.

I also noticed in my testing that entering the correct answer (-1/3)exp(-5x) was marked incorrect (your results may vary depending on the random points chosen). This is due to the fact that the checker is checking whether a formula is identically zero. Testing against zero is always touchy, as it involves the zero level and zero-level tolerance. These are set to work for numbers that are computed from values that are on the order of magnitude of 1, but don't work well for other magnitudes. In your case, with exp(-5x) involved, with the default range for x of -2 to 2, you can get answers with magnitude over 7000 from (-1/3)exp(-5x). That means that the round-off error that the zero-level tolerance is trying to capture will now be 4 decimal places higher, and so the default tolerance will say that those errors are not small enough to be counted as zero. That is what happened in my case for one of the test points.

One either needs to adjust the zero-level tolerance, or to do the test differently. Rather than testing if $Y2 + 3 * $Y1 - 4 * $Y0 - Formula('-2 exp(-5x)') is zero, you could test whether $Y2 + 3 * $Y1 - 4 * $Y0 equals Formula('-2 exp(-5x)') instead. That would avoid the problem of zero levels. It is always best to avoid testing against zero when possible. So a better version of the checker might be:

ANS( $ans6c->cmp( 
  bypass_equivalence_test => 1,
  checker => sub {
      my ( $correct, $student, $ans ) = @_;

      return $correct == $student if $ans->{_filter_name} eq 'Answer Hints Post Filter';
      return 0 if $student->type eq 'String';

      # Try to see if it satisfies the ODE
      $Y0 = Formula( $student );
      $Y1 = $Y0->D('x');
      $Y2 = $Y1->D('x');

      $toCheck = $Y2 + 3 * $Y1 - 4 * $Y0 ;

      return 1 if Formula('-2 exp(-5x)') == $toCheck;
      $ans->{ans_message} = "That is not a solution of the non-homogeneous equation" unless $ans->{isPreview};
      return 0;
  }

)->withPostFilter(AnswerHints(
  Compute("DNE")  => 'There is a solution.', 
  Formula('sin(x)') => [ 'Junk message - sin(x).', replaceMessage => 1 ],
  Formula('exp(x)') => [ 'Junk message - exp(x).', replaceMessage => 1 ],
))

I agree with @somiaj in his comment that if you are using a checker that produces messages already, you might as well produce the other messages in the checker rather than using AnswerHints as well. But you can make it work if you use replaceMessage options on all the AnswerHints messages.

There is another approach, however, that avoids the need for replaceMessage, which puts off setting the answer message until after AnswerHints runs. Here is one way to do it:

loadMacros('PGstandard.pl', 'MathObjects.pl', 'answerHints.pl');

Context("Numeric");

$ans6c = Compute('(-1/3)*exp(-5x)'); # Needs grader as solution of non-homogeneous ODE

Context()->texStrings;
BEGIN_TEXT
Find a solution to:
\[ y'' + 3 y' - 4 y = -2 e^{-5x} \]
\{ $ans6c->ans_rule(10) \}
END_TEXT
Context()->normalStrings;

ANS( $ans6c->cmp( 
  bypass_equivalence_test => 1,
  checker => sub {
      my ( $correct, $student, $ans ) = @_;

      return $correct == $student if $ans->{_filter_name} eq 'Answer Hints Post Filter';
      return 0 if $student->type eq 'String';

      # Try to see if it satisfies the ODE
      $Y0 = Formula( $student );
      $Y1 = $Y0->D('x');
      $Y2 = $Y1->D('x');

      $toCheck = $Y2 + 3 * $Y1 - 4 * $Y0 ;

      return 1 if Formula('-2 exp(-5x)') == $toCheck;
      $ans->{non_solution} = 1;
      return 0;
  }

)->withPostFilter(AnswerHints(
  Compute("DNE")  => 'There is a solution.', 
  Formula('sin(x)') => 'Junk message - sin(x).',
  Formula('exp(x)') => 'Junk message - exp(x).',
))->withPostFilter(sub {
  my $ans = shift;
  $ans->{ans_message} = "That is not a solution of the non-homogeneous equation"
     if $ans->{non_solution} && !$ans->{isPreview} && !$ans->{ans_message};
  return $ans;
}));

So there are several ways to address this issue within the checker itself.

If you all do decide to make this change, I'd recommend moving the test to before the loop over the wrong list, since it gets used by both conditions in the code below. It would also be good to move the answer message check to above the loop as well, since it is in both conditions, too.

I agree that the documentation for AnswerHints needs improvement, in particular, that the checkTypes and checkCorrect options only apply to CODE answers, and that if a custom checker is provided, it will be called with the AnswerHints answers as the $correct answer, so the checker needs to take $correct into account in order to process those.

[taniwallach]

@dpvc - Thanks for all the feedback, especially the advice about avoiding the comparisons to 0. I'll need to run over quite a number of questions and make the suggested improvements, also ODE questions without any use of answer hints. BTW the messages about sin(x) and exp(x) were just samples to show that under the circumstances the first message would be triggered. In one of the questions where I ran into trouble on 2.19 I was getting messages which were intended to be triggered for more complicated "wrong answers" which were expected to be checked by answerHints only if the original answer was not correct. I'm not sure if they were actually being triggered when needed in the past, but the first one in the list was certainly were getting triggered incorrectly for correct answers on 2.19.

For now, I'm leaving my server running the "patch" as it at least prevents correct answers from getting "hints" incorrectly until I can work on revising all the potentially effected problems.

I'm still not that happy with the change to PG "breaking" problems that worked "well enough" before, but since it is certainly an edge case, and a non-standard use of answerHints - I guess that it makes sense to make answerHints work as you recommend and to close this PR and instead remark on the matter in the release notes and work to improve the documentation and sample problems.

I'm not closing the PR for now, so it can be a reminder to work on the documentation matters.

[dpvc]

I'm still not that happy with the change to PG "breaking" problems that worked "well enough" before

The old problems were broken, as the answer hints were never working properly. You just weren't noticing it, but they were never doing what they were supposed to. That may be "well enough", but I would think that being able to identify and fix them is more important than giving the impression that they are working and that there are functional hints.

I explained above that if the checker function doesn't actually involve the $correct answer, then it will never properly detect the answers in the answer-hints list. What would happen is that if the student answer is correct, any answer in the list would match as correct, and if the student answer were incorrect, no answer would match. Whether those tests were being made depends on whether there was an answer message produced by the checker, and (in the past) by whether the student answer is correct.

In your case, if the student answer was correct, no answer-hint checks were made, and so there would be no answer hints, but if the student answer was incorrect, then no answer-hint answer would match, so again, not answer hints would be made. Also, in this case, the checker produced an answer message itself, which also blocked checking of answers. The only exception was when the student answer was DNE, in which case the checker dies early when trying to differentiate a string, and so no answer message is provided, but the answer himt also fails to be produced since the checker fails to match the DNE answer (it still tries to differentiate the student answer of DNE, and so the checker doesn't match the hint answer of DNE).

The upshot is that the answer hints for that problem would never be produced under any circumstances, as far as I can see, with your change here. With the current version of AnswerHints, the difference is that when the student answer is correct, then all the hint answers are going to match as correct, so the first answer hint will be given, as you have seen.

While I understand the desire not to change the behavior of working problems, and agree with trying to maintain backward compatibility whenever possible, making other people have to add checkCorrect options on correct answers given explicitly in the answer hints list in order to preserve the non-functionality of answer hints in problems like this seems like a bad trade-off to me.

[dpvc]

PS, sorry if that came off as harsh, but I was just trying to be clear.

[taniwallach]

PS, sorry if that came off as harsh, but I was just trying to be clear.

Thanks - it was pretty clear before. It was a petulant of me to complain about the change breaking "my" problems when they certainly were not really correctly coded.

I was aware that the hints in these problems did not work properly (there were comments about that in several of them) but did not know why at the time. I had not understood that answerHints used the custom checkers when "checking" against the list of "answers" to trigger messages, and had not considered that this would be the case.

Bottom line - the "rush" to move from my old system to WeBWorK when these problems were coded created "technical debt" of various sorts, and this is one case where this has become a pretty serious issue for content on my system.

I am grateful that the "true issue" is now clear, and hopefully both I and other in the future will be able to be lazy and sue answerHints together with custom grades in a proper manner, when that seems a decent idea. And yes, I agree in than in most cases it makes more sense to just add the messages directly in the custom grader.