Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show Me Another Refactor #612

Merged
merged 12 commits into from
Oct 22, 2015
Merged

Show Me Another Refactor #612

merged 12 commits into from
Oct 22, 2015

Conversation

goehle
Copy link
Member

@goehle goehle commented Jul 22, 2015

This refactors the Show Me Another to be its own content generator page inheriting the elements of the Problem content generator page. In particular most of the Show Me Another elements have been removed from Problem and put into their own perl module. Test by checking to see that show me another still works as intended.

  • Check to see that you can enable and disable the system.
  • Check to see that you can enable and disable SMA for individual problems.
  • Check to see that the problem and system wide options for SMA all do what they are supposed to do.
  • Check that SMA is enabled and disabled appropriately.
  • Check that SMA is not generating problems which are the same as the original problem.

@jwj61 jwj61 force-pushed the develop branch 2 times, most recently from 1adc11c to 484f28b Compare August 4, 2015 17:02
@goehle
Copy link
Member Author

goehle commented Aug 26, 2015

Note, this should be pulled into develop after 2.11 is split off.

…o smaref

Conflicts:
	lib/WeBWorK/ContentGenerator/Problem.pm
@goehle
Copy link
Member Author

goehle commented Aug 31, 2015

I merged develop into this branch. The conflicts with problem got kind of hairy so you should also check:

  • Check functionality of Problem Page
  • Check functionality of Problem Page for JITAR problems.

@Alex-Jordan
Copy link
Contributor

So ultimately this has been pulled into develop before 2.11 is split off? I'm looking for things that I can help test. If this is to be part of 2.11, I might put it on that list. If it's coming in later, it's less urgent.

@goehle
Copy link
Member Author

goehle commented Sep 2, 2015

This is probably better not being in 2.11. If you want to test something,
you can test the installer.

On Tue, Sep 1, 2015 at 10:04 PM, Alex Jordan notifications@github.com
wrote:

So ultimately this has been pulled into develop before 2.11 is split
off? I'm looking for things that I can help test. If this is to be part of
2.11, I might put it on that list. If it's coming in later, it's less
urgent.


Reply to this email directly or view it on GitHub
#612 (comment).

@Alex-Jordan
Copy link
Contributor

I see, I misunderstood "I merged develop into this branch" backwards.

We use SMA a lot, and there is one common piece of feedback instructors give me. Since this branch is a major SMA restructuring, I think I will comment about it here. But these comments apply to develop right now, not to any work that has already been done with this branch.

Basically, it's too hard/tedious to set the SMA threshold numbers (the number of tries it takes for the SMA button to unlock). This is a problem-by-problem parameter. It's real original purpose was just so you can set it to -1 to turn SMA off on certain problems, but we increased its functionality by making it an attempt threshold for unlocking the button.

If an instructor imports a set from a .def (V1) where all of the SMA threshold numbers were blank, then the value of $problemDefaults{showMeAnother} is written to the database for each problem. If an instructor imports a set from a .def (V2) where all of the SMA threshold numbers were defined, then those numbers are written to the database entry for the problem. (I haven't explored a V1 .def where all/some of the SMA threshold numbers are defined.)

At this point, since numbers have been written to the database, the only thing an instructor can do to change these threshold numbers is to manually change each one for each problem. This scenario is bad when they are using a .def file they inherited from someone else. (If it's V1 and the SMA threshold numbers are blank, and if they know they can set $problemDefaults{showMeAnother} in course.conf, then that helps, but most faculty don't know to do that. But it doesn't help after the import, and it doesn't help with V2 problem .defs, which have mostly hard-coded whatever $problemDefaults{showMeAnother} was set to on the previous user's course.)

One idea to remedy this is as follows:

  • make the global default SMA threshold ($problemDefaults{showMeAnother}) something you can set in Course Config, alongside the other SMA parameter (that caps how many times SMA can be used per problem)
  • in each problem's space in the database, allow for showMeAnother = 'inherit', meaning to use whatever the global value is.
  • When a V1 .def with no defined thresholds is imported, write 'inherit' to the problem's slot in the database rather than using the value of $problemDefaults{showMeAnother}
  • As more V2 .defs are created over time, most of the time they will have their showMeAnother = 'inherit'. For some problems, there will have been manual changes to showMeAnother = -1, showMeAnother = 0, etc.
  • Instructors can then change the global default in their Course Config, and instantly see most of their assigned problems use that threshold. Only the ones that someone intentionally set to something other than 'inherit' will remain unchanged.

Once this branch is merged into develop later, if what I am talking about is still an issue, I can volunteer to implement what I have described here. Assuming there isn't something bad about it someone sees.

@goehle
Copy link
Member Author

goehle commented Sep 3, 2015

I'm not fond of having more magic keywords floating around. It also doesn't necessarily solve your problem of someone exporting a set with -1's for the SMA field and having that set imported by someone who wants to then use SMA. The -1's will still be put into the database and you will still have to change things manually. Or am I misunderstanding?

@Alex-Jordan
Copy link
Contributor

If preferred, instead of 'inherit', just leaving that database entry blank could serve the same purpose. It just probably violates some principle of coding to purposely leave an entry blank. Or -2 could take that roll, but then we start having more cryptic numerical codes that serve purposes.

In what I describe, values other than 'inherit' (or empty, or -2) would not be put into the database very often.

  • If Alice wants SMA off altogether, she turns it off with Course Config. Her sets all export with inherit in the threshold field for each problem
  • If Alice wants SMA off on most problems, but turned on for a few, she sets $problemDefaults{showMeAnother}=-1 in Course Config. Most problems have inherit in their database field. She uses the Hmwk Sets Editor to change some of them to 0, 2, or whatever. When she exports a set, it has a bunch of inherits, and a few 0s, 2s, etc in the threshold fields for the problems.
  • If Bob imports her .def file, it's still the case that most field entries are inherit in Bob's database. Bob can just change his $problemDefaults{showMeAnother} in Course Config to affect these problems. Bob may want to manually change a few values, including any that Alice had set.

And all this can work the other way too, where Alice uses SMA a lot and has the threshold at 0 most of the time, but Bob only wants to use it here and there.

@goehle
Copy link
Member Author

goehle commented Sep 3, 2015

It would have to be a magic number. The "words" that go there are just sugar added by ProblemSetDetail. And I suppose you would have to add that as well, although I would call it "default" and not "inherit". You should also put a tool tip next to the box to indicate what all of these magic words mean.

The only other solution I can think of is to only allow a course wide settings. SMA is either on for the whole course with a certain set of parameters or off for the whole course. Why do we need to turn SMA off for certain problems again?

@Alex-Jordan
Copy link
Contributor

| Why do we need to turn SMA off for certain problems again?

The biggest reason I recall is you might mostly want it on, but turn it off for multiple choice questions. If the multiple choice question scrambles the order of options, it will be enough to count as a new version even if the options are the same. So it gives away the answer to the question that's for credit.

@goehle
Copy link
Member Author

goehle commented Sep 3, 2015

Ah. Yeah, this was always the fundamental danger of SMA. I suppose we have a choice:

Option 1: Make Show Me Another a global option. Its mostly global right now. The only per problem option is the number of attempts to make the button available. The benefit is the whole system is simpler. The downside is that there are problems where information leakage could occur with SMA.

Option 2: Allow for a "default"/"inherit" value for the number of attempts to make show me another available. The upside is you can now allow each problem to have individual values if you want, but you can also set things up so that they get the default value. The downside is its more complex and means we have an extra problem parameter which may not be used much.

@cmhughes
Copy link
Contributor

cmhughes commented Sep 4, 2015

I think the main reason we wanted SMA to be configured on a problem by
problem basis is that some problems change with the problem seed, and some
do not; it depends on of the problem contains randomisation or not.

I thought that I had coded a default value along the lines of the inherit
option you're describing, which was set up in one of the config files.

PS: very happy to see it receiving some more attention!

On Thursday, September 3, 2015, Geoff Goehle notifications@github.com
wrote:

Ah. Yeah, this was always the fundamental danger of SMA. I suppose we have
a choice:

Option 1: Make Show Me Another a global option. Its mostly global right
now. The only per problem option is the number of attempts to make the
button available. The benefit is the whole system is simpler. The downside
is that there are problems where information leakage could occur with SMA.

Option 2: Allow for a "default"/"inherit" value for the number of attempts
to make show me another available. The upside is you can now allow each
problem to have individual values if you want, but you can also set things
up so that they get the default value. The downside is its more complex and
means we have an extra problem parameter which may not be used much.


Reply to this email directly or view it on GitHub
#612 (comment).

@Alex-Jordan
Copy link
Contributor

@cmhughes I think that you had coded that way. More specifically, I think that you had it so that database entries for the threshold for each problem were left blank when there was nothing in the .def file to use. And so then it all relied on the global parameter. Over the past year, I could tell faculty to add a line to their course.conf to generally change all of their SMA numbers, as long as none had already been written to the database.

But now that I have pulled develop, this is no longer the case. It appears that now when you import a .def where the SMA thresholds are blank, then it is taking the global value and writing it to the database for each problem. So at that point if you change the global value, it's too late. And if you export the set, the resulting .def will have the global threshold for each problem instead of blanks. I think this may have to do with implementing the new "V2" format for .def files.

@goehle
Copy link
Member Author

goehle commented Sep 4, 2015

Its not really problem list v2 or v1. I think you will have the same behavior with both. Its the line
https://github.com/openwebwork/webwork2/blob/develop/lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList2.pm#L2122
which is

 unless ($showMeAnother =~ /-?\d+/) {$showMeAnother  = $showMeAnother_default;}

This is the line that is setting $showMeAnother to be the default value. If you really want a NULL database entry to mean the default then you just delete this. However, if you do that then users will never be able to get back to the default if they enter a value.

@Alex-Jordan
Copy link
Contributor

This line is in develop, and not in master. So that explains why I am seeing a behavior change.

@goehle
Copy link
Member Author

goehle commented Sep 4, 2015

Would reverting the behaviour back to the way it is in master be appropriate for 2.11?

@Alex-Jordan
Copy link
Contributor

Hard to say. I'm not a great software engineer, but it feels wrong to
purposefully leave null entries. It's the kind of thing that can leads to
error messages down the road if, say perl changes its behavior to become
more fussy with comparisons involving null.

I'd rather get some more opinions first. In any case, I can volunteer to
operate on it after 2.11, and after this restructuring is merged into
develop. I'm still a fan of the "-2" option for default.

On Fri, Sep 4, 2015 at 1:47 PM, Geoff Goehle notifications@github.com
wrote:

Would reverting the behaviour back to the way it is in master be
appropriate for 2.11?


Reply to this email directly or view it on GitHub
#612 (comment).

Alex Jordan
Mathematics Instructor
Portland Community College

@goehle
Copy link
Member Author

goehle commented Oct 7, 2015

Made changes suggested by Alex, there is now a "showMeAnotherDefault" option in the configuration menu and if the show me another value for a problem is -2 then this default is used. The -2 is also used as the default value for newly created problems. Things to test:

  • Import a set without expressly defined show me another values and check that the default -2 is set.
  • Check that if the problem sma field is set to -2 then the course default is used, but can be overridden by problem set detail.
  • Check that the course wide default can be changed using the configuration menue.
  • Check that SMA still works as intended.

@mgage
Copy link
Sponsor Member

mgage commented Oct 21, 2015

on this problem: Library/ma117DB/set1b/srw1_3_77.pg

show me another button is shown but I get the response: WeBWorK was unable to generate a different version of this problem; close this tab, and return to the original problem.
*https://hosted2.webwork.rochester.edu/webwork2/gage_test/showMeAnother/2/)
-- is this standard? -- random is used in the problem
same with https://hosted2.webwork.rochester.edu/webwork2/gage_test/showMeAnother/5

https://hosted2.webwork.rochester.edu/webwork2/gage_test/showMeAnother/3/
-- when I go to "show me another" the answer blanks have been auto filled with the answers to the previous problem -- not a big deal, but is this what we want?

@mgage
Copy link
Sponsor Member

mgage commented Oct 21, 2015

when I go to the show me another button I see that I have attempted the problem 1,001 times.
Can we adjust the wording here? perhaps enter the number of attempts on the original problem?

@mgage
Copy link
Sponsor Member

mgage commented Oct 21, 2015

this seems to work correctly with minor caveats. Is there someway to detect in advance whether a "show me another" problem can be created? It's a little frustrating if there are several problems that display a "show me another button" but then give the message that the problem can't be created when you press it.

For the instructor it might not be clear as to why a different problem can't be created. (see examples above)

@goehle
Copy link
Member Author

goehle commented Oct 21, 2015

Addressing the easy ones first:

  • I think we probably should turn off the autofill for the previous answer for show me another. This should be easy enough to do in the new setup.
  • The 1001 attempts thing forces hints to be shown. In order to change it you would need to have an override in PG which shows hints no matter how many attempts. (The library browser uses this same technique when you view a problem.) If something like that is already there then we could use it. (Something similar to the instructor override but without the instructor message.)
  • The "unable to generate another problem" message was a messy compromise from the original pull request. The way the system works is it picks a new seed and renders the problem with the original seed and the new seed. Then it compares problem body for the two and if they are not different it tries a new seed. It does this some number of times before it gives up. You could do this every time the Problem page loads and disable the show me another button if it fails, but that would at least double or the server workload and was considered too expensive. Another option would be to refactor the system so that alternate show me another seeds are created at the time of set generation and stored in the database. This would make generating sets more expensive, and would mean adding a database column, but it would solve the "detecting in advance" problem without increasing the load of Problem.

@Alex-Jordan
Copy link
Contributor

The way it works is the HTML for the potential new version of the problem
is compared to the HMTL for the original.

If there is any difference (even if the two problems have the same answer)
it is supposed to be a go. After some number of tries to find a new HTML
(using random seeds) if it fails all tries, it gives up.

Looking for this ahead of time is problematic because each load of a
problem would require lots of PG compilations to check all the HTML
outputs.

We wanted it to work this way because:

  • You don't want the SMA to be identical to the for-credit version and
    give away the answer.
  • If you just use a new seed, you might well end up with the same
    problem, even if the problem uses randomization.
  • If you base the comparison on different answers instead of different
    questions, then you might rule out too much. Since lots of different
    versions of a question can have the same answer.

I can't look at that problem right now, but are you sure that even though
it may have a randomization perl command, that it actually leads to
different questions?

It's perhaps tedious, but if you know SMA will fail like this on a problem,
you can set the SMA threshold to -1 in the Hmwk Sets Editor.


I'd like to make the filled-out answer blanks go away. That is a definite
negative, when I tell students to use SMA for more practice.

Agreed on 1001.

On Tue, Oct 20, 2015 at 5:42 PM, Michael Gage notifications@github.com
wrote:

this seems to work correctly with minor caveats. Is there someway to
detect in advance whether a "show me another" problem can be created? It's
a little frustrating if there are several problems that display a "show me
another button" but then give the message that the problem can't be created
when you press it.

For the instructor it might not be clear as to why a different problem
can't be created. (see examples above)


Reply to this email directly or view it on GitHub
#612 (comment).

Alex Jordan
Mathematics Instructor
Portland Community College

@mgage
Copy link
Sponsor Member

mgage commented Oct 21, 2015

Let’s fix the autofill and leave the other two as is. Neither is fatal and you get used to it
pretty quickly. I suspect that the problems I chose don’t produce much variation
Library/ma117DB/set1b/srw1_3_77.pg
Library/NewHampshire/unh_schoolib/Factoring/facbrs201.pg

Could we just turn this part off on the show_me_another_page?
You have attempted this problem 1,001 times.
Your overall recorded score is 100%.
You have unlimited attempts remaining.

I’m not sure that info is necessary on the practice page.

Take care,

Mike

On Oct 20, 2015, at 9:16 PM, Geoff Goehle notifications@github.com wrote:

Addressing the easy ones first:

I think we probably should turn off the autofill for the previous answer for show me another. This should be easy enough to do in the new setup.
The 1001 attempts thing forces hints to be shown. In order to change it you would need to have an override in PG which shows hints no matter how many attempts. (The library browser uses this same technique when you view a problem.) If something like that is already there then we could use it. (Something similar to the instructor override but without the instructor message.)
The "unable to generate another problem" message was a messy compromise from the original pull request. The way the system works is it picks a new seed and renders the problem with the original seed and the new seed. Then it compares problem body for the two and if they are not different it tries a new seed. It does this some number of times before it gives up. You could do this every time the Problem page loads and disable the show me another button if it fails, but that would at least double or the server workload and was considered too expensive. Another option would be to refactor the system so that alternate show me another seeds are created at the time of set generation and stored in the database. This would make generating sets more expensive, and would mean adding a database column, but it would solve the "detecting in advance" problem without increasing the load of Problem.

Reply to this email directly or view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openwebwork_webwork2_pull_612-23issuecomment-2D149750914&d=BQMCaQ&c=kbmfwr1Yojg42sGEpaQh5ofMHBeTl9EI2eaqQZhHbOU&r=C6Pt5AGtImanmAdcooarL-JZO8M5dSFPfs3VweYXYkE&m=TdMHWttZCpw8unXiwfZaWgF-EA4UPdYKOyCy8bs05lA&s=0fyD-QT_QE7zCudJsRhWulQSo4ILYCtuJvK0zb-ucTQ&e=.

@goehle
Copy link
Member Author

goehle commented Oct 22, 2015

I made the two requested changes.

@mgage
Copy link
Sponsor Member

mgage commented Oct 22, 2015

OK. This looks good. I'm pulling it.

mgage added a commit that referenced this pull request Oct 22, 2015
Show Me Another Refactor
@mgage mgage merged commit 346eda7 into openwebwork:develop Oct 22, 2015
@goehle goehle deleted the smaref branch January 15, 2016 02:05
@goehle goehle mentioned this pull request Apr 20, 2016
41 tasks
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.

None yet

4 participants