Skip to content
This repository has been archived by the owner on May 11, 2021. It is now read-only.

Multiple question with a single custom item without answer #161546

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wrwrwr
Copy link

@wrwrwr wrwrwr commented Nov 15, 2015

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:

  • basic_set_notation.html,
  • ellipse_intuition.html,
  • graphing_inequalities_2.html,
  • graphing_points_2.html,
  • graphing_systems_of_equations.html,
  • graphing_systems_of_inequalities.html,
  • hyperbola_intuition.html,
  • parabola_intuition_3.html,
  • unit_circle.html,
  • visualizing_derivatives.html.

(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?

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.
@wrwrwr
Copy link
Author

wrwrwr commented Nov 16, 2015

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 :-)

@itsjohncs
Copy link
Contributor

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.

@wrwrwr
Copy link
Author

wrwrwr commented Nov 21, 2015

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) {
Copy link
Contributor

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?

Copy link
Author

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.

@itsjohncs
Copy link
Contributor

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.

@wrwrwr
Copy link
Author

wrwrwr commented Dec 6, 2015

There are 3 "infrastructural" changes on the branch:

  • First, turn the empty-string message from unanswered custom problems into a null. Exactly as here (except for a better commit message ;-) As it turns out this is not only needed for custom-in-multiple exercises, but also in any case that a custom problem should be unanswered despite having a neither-empty-string-nor-array-of-empty-strings guess. For instance test-simple-custom.html should be treated as unanswered, not incorrect.
  • Second, refactor the multiple validator to make partially-correct, partially-unanswered exercises unanswered. Consider test-simple-multiple.html. What should happen if I correctly fill just one of the fields and leave the other empty? I'd say "more parts to answer" should be shown and the exercise should not be graded as incorrect. Basically, that's like changing the and on line 1220 to an or, except that you need to take care that the right message is passed on (in case there is more than one).
  • Third, make radio-in-multiple work. There are two pieces of it: -- the radio has choices outside of its solution element, which multiple unnecessarily clones into the solution area; -- there is a discrepancy between what createValidator() gets from multiple setup() and what it expects in the radio case. Here is a minimal test case test-radio-in-multiple.html.

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.
Copy link
Contributor

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.

Copy link
Author

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).

@xymostech
Copy link
Contributor

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.

@xymostech
Copy link
Contributor

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!

@wrwrwr
Copy link
Author

wrwrwr commented Dec 17, 2015

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants