-
Notifications
You must be signed in to change notification settings - Fork 865
Multiple question with a single custom item without answer #161546
base: master
Are you sure you want to change the base?
Conversation
The validator function now returns an empty string if the user does not modify the graph or select a radio.
When a custom problem is not answered, the validator is expected to return an empty string result. The "score" is then marked as "empty". Instead of passing the empty string as "message" further into the framework it should be better to convert it to null right away. This seems cleaner than handling it in the multiple-validator, and makes custom exercises also display "more parts to answer" when a user just clicks "Check Answer" without doing anything.
Today's commit also influences (a bit) how custom exercises work, although I believe that's the way to go, some additional testing would be advisable :-) |
Awesome! Thanks for following up with this. I'm currently a little swamped making some new changes to another part of our site, but I'll take a look through this code next week. |
The multiple-exercise unanswered parts handling change I was thinking about could look something like master..unanswered-multiple-and-custom. If you do get to this at some point (be it next week or some other), please take a look at that branch (especially the test changes). If that makes sense, it would be better than what we have here. |
@@ -206,7 +206,7 @@ | |||
$("input[name='isSolution']:checked").attr("id")] | |||
</div> | |||
<div class="validator-function"> | |||
if (_.isEqual(guess, [[-5, 5], [5, 5], false, true])) { | |||
if (_.isEqual(guess.slice(0, 4), [[-5, 5], [5, 5], false, true]) || guess[4] === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this have ever fired without that slice there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case the guess has 5 elements, so no. A simpler fix is to add null
as the fifth element to the array, but this is still not how I'd see it.
Logically, this exercise consists of two parts, a graph and a radio selection. It would be nice to reflect this in its structure. I imagine it could look something like this:
<div class="solution" data-type="multiple">
<div class="sol" data-type="custom">...</div>
<div class="sol" data-type="radio">...</div>
<ul class="choices" data-category="true">...</ul>
</div>
This is what drove the changes on the other branch (including making the problem unanswered when no part or just one part is answered). At least the two changes related to radio-in-multiple (the third commit) are needed to make it work.
This is looking sweet! I asked some questions inline to make sure I'm understanding things, but this seems pretty solid to me. It's a bit frightening because of all the exercises it touches, but this seems like it should be pretty safe. I'll poke @beneater as well though to see if he has any concerns I haven't thought about. And I'm sorry it was longer than I expected for me to get to this. I hadn't realized it had been so long since my last message :(. And what's the approach your taking in master...wrwrwr:unanswered-multiple-and-custom? I took a quick look but couldn't get a sense of the high level changes by thumbing through the code. |
There are 3 "infrastructural" changes on the branch:
The first three commits are independent -- we could probably split them into three pull requests. The last three commits modify one exercise each and depend on the architectural changes -- these result from looking through some exercises to check if anything could / should be changed with respect to the architectural changes (so far just 30 out of over 150 exercises that may be affected). |
@@ -1216,6 +1216,7 @@ Khan.answerTypes = $.extend(Khan.answerTypes, { | |||
// otherwise correct or not before forwarding on the | |||
// message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here should probably be changed to reflect the new semantics of what is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be still valid (the unsimplified fraction is the only case that the branch is taken that I'm aware of).
If a student enters "2/4" into a proper fraction field a "your answer is almost correct" message is normally displayed. (Under the default mode of simplification.) Such an answer does not fall under any of the three usual categories of "empty", "correct", or "incorrect", thus the framework uses a special score for it -- the score is marked as "empty", but carries the message.
Now the multiple exercise has a surprising semantic: if any subscore is "empty", but not all are, then the combined score is incorrect (even if all nonempty parts are actually correct; this happens as empty scores have correct: false
). Without the branch with the lengthy comment, any multiple with an unsimplified part and another correct part would be graded wrong.
If we were to let multiple be empty whenever at least one of its parts is empty and all nonempty parts are correct, then this special case would be unnecessary -- a single unsimplified fraction would cause the combined score to be empty. And an empty score is treated as unanswered, displaying the message it carries. That's the content of wrwrwr@9a25e47 (not part of this PR).
Hello! @brownhead is a bit busy with other stuff, so I'm helping out. All of these changes look good! And thank you for writing some tests, we have a severe lack of them in khan-exercises at the moment. I'm going to check this out and play with things to make sure I'm confident about what's going on, but the code looks solid. |
I tried out graphing_inequalities_2 and unit_circle, and they both worked as expected. Can you look over the comment I comment on and make sure it matches up with your understanding of what's going on? Thanks! |
Do we want to go with the minimal changes here or dare to try the more semantic approach from the branch? In any case feel free to copy-and-paste, cherry-pick, branch etc. any part of it -- whatever feels most convenient. |
Second try at #161538.
When a multiple-type problem contains a single custom item, and the user clicks "check answer" without doing anything (thus the custom validator function likely returns an empty string) with the change KE says that there are parts of the problem left to answer rather than grading as incorrect.
For reference, the changed piece of code was introduced in d4288db. There are couple of custom-in-multiple exercises, namely:
(Note: A few of these contain more than a single custom item -- see below.)
This is pretty minor, not sure if it's worth the time for reviewing, testing, etc. given the status of the legacy KE -- feel tree to close it altogether if that's the case. On the other hand, the multiple-kind exercise currently considers a guess (answer) empty (that is unanswered) only if all its parts are empty (rather than any one of its parts). This seems neither convenient nor consistent with the new framework. Should we change it?