Skip to content

Patch AnswerHints#680

Merged
pstaabp merged 4 commits intoopenwebwork:PG-2.17from
taniwallach:answerhints-patch
Jun 5, 2022
Merged

Patch AnswerHints#680
pstaabp merged 4 commits intoopenwebwork:PG-2.17from
taniwallach:answerhints-patch

Conversation

@taniwallach
Copy link
Member

Patch AnswerHints so that if a subroutine in the list "dies" there will still be a final result from the AnswerHints postfilter.
This is mean to address #679.

still be a final result from the AnswerHints postfilter.
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is okay.

I have mixed feelings about this though.

Your example code in the linked issue should set $tmp = 0 and then wrap the $tmp = $student->eval(x => 0) in a perl eval. Then the answerHint macro will work as expected and give the message that the condition requested in the problem is not satisfied. This pull request will hide this error, and make it harder to see that the hints are not working in the intended way.

On the other hand, it doesn't hurt to catch errors from special cases don't matter that are accidentally missed by the author.

Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the error in the posted problem. I would recommend the cleanups that @drgrice1 pointed out.

@taniwallach
Copy link
Member Author

@drgrice1 @pstaabp - Thanks. Perltidy changes are in the third commit.

Your example code in the linked issue should set $tmp = 0 and then wrap the $tmp = $student->eval(x => 0) in a perl eval. Then the answerHint macro will work as expected and give the message that the condition requested in the problem is not satisfied.

Thanks. I fixed the real problem (was more detailed, but the core error was the occurrence of a divide by zero error for a student answer at the initial condition point). I had not considered the need to such until this occurred.

I thought it would be helpful to document this in the Wiki, but the Wiki problems technique page about AnswerHints does not even mention the use of a subroutine test, so explaining there that evaluation at a test point could potentially lead to such an issue seems overboard there.

This pull request will hide this error, and make it harder to see that the hints are not working in the intended way.

Good point. I added a fixed "error message" (as a result table message) for when the eval fails. That will make the existence of a problem visible to the end user (who may report it) but prevents the post filter from dying and messing up the allocation of weighed credit, etc.

I tried unsuccessfully to use the value of $@ to retrieve the internal error message ("Divide by zero"). Using warn ref($@); could get that to show up as a warning message at the bottom, but I did not manage to get it out to put into the results table. It seems my understanding of what a failed eval is putting into $@ is just not sufficient to dig it out. Ideas for how to do that are welcome.

@taniwallach taniwallach requested a review from drgrice1 May 26, 2022 13:59
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The perltidy result is a little different with the remove options added back in, but it will do for now.

I am not sure that I agree with displaying the "error during AnswerHints processing" as an answer message. I think of those messages as informative messages for the student. I think it might be better if that were a simple warn statement so that it appears with the information about reporting the issue to the professor and such. This is really an issue with the problem. In any case, this is will do for now also.

I am not sure why $@ is not being set exactly. It most likely has to do with the $SIG{__WARN__} handling inside the translator though. The way that you dealt with it seems to be working well though.

@pstaabp
Copy link
Member

pstaabp commented May 29, 2022

This looks good now. I agree with @drgrice1 about the message. We could say something along the lines of "An error occurred in this problem. Please inform your instructor."

@taniwallach
Copy link
Member Author

This looks good now. I agree with @drgrice1 about the message. We could say something along the lines of "An error occurred in this problem. Please inform your instructor."

Sounds good to me, but I would like to somehow mention AnswerHints in the message, to provide a hint to whoever debugs it of where to look. How about: "An error occurred in this problem. Please inform your instructor. (die in AnswerHints subroutine)"?

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an issue with the eval approach. See my comment.

AnswerHints test "wrong" subroutine used to trigger hints returns
a zero.
Change the text of the error message issued when the "eval" fails, and
make it a "warn" instead of a results table "answer message".
@taniwallach
Copy link
Member Author

I am not sure that I agree with displaying the "error during AnswerHints processing" as an answer message. I think of those messages as informative messages for the student. I think it might be better if that were a simple warn statement so that it appears with the information about reporting the issue to the professor and such.

Upon additional consideration, I changed the error message to a warn and the text to "An error occurred in this problem."

Since it is now a warn it will trigger the standard message above the problem: "Warning -- There may be something wrong with this question. Please inform your instructor including the warning messages below." Since the warn is from inside answerHints.pl the message below the problem provides an indication to the staff where to look for the bug.

@taniwallach taniwallach requested a review from pstaabp June 2, 2022 07:17
@pstaabp pstaabp merged commit 3a3258c into openwebwork:PG-2.17 Jun 5, 2022
@taniwallach taniwallach deleted the answerhints-patch branch June 6, 2022 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants