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

Fix problem with correct_ans being incorrectly set #261

Merged
merged 1 commit into from Dec 14, 2015

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Dec 14, 2015

This patch uses of the noInherit list to remove unwanted fields from propagating to copies of existing objects. This affects the MathObject's Value class' make() method (and the overridden ones in the Complex and Interval objects). This means that it is fairly far reaching, so we should keep an eye out for unexpected consequences, though I don't think there should be any.

This resolves bug 3550, and the unreported but similarly caused problems in Complex and Interval objects.

To test, use

$f = Compute("sin(x)");
$a = Compute("pi");
$b = $f->eval(x=>$a);

BEGIN_TEXT
\(\sin(\pi)\) = \{$b->ans_rule(5)\}
END_TEXT

ANS($b->cmp());

Without the patch, the correct answer will show up as pi rather than the correct value of 0. With the patch, the correct value should be shown.

To test the Complex object, use

Context("Complex");
$f = Compute("sin(z)");
$a = Compute("pi+i");
$b = $f->eval(z=>$a);

BEGIN_TEXT
\(\sin(\pi+i)\) = \{$b->ans_rule(5)\}
END_TEXT

ANS($b->cmp());

and for the corresponding Interval situation, use

Context("Interval");
$I = Compute("[1,5]")->new(2,3);

BEGIN_TEXT
\((2,3)\) = \{$I->ans_rule(10)\}
END_TEXT

ANS($I->cmp());

Here, make sure the correct answer shows as (2,3) rather than [1,5].

…, are not copied from one object to another during an eval() call.
@goehle
Copy link
Member

goehle commented Dec 14, 2015

  • I'm having trouble getting your third test case working. I get 2,3 (no parens) as the correct answer before and after the patch. Edit: Don't worry about this, I figured it out.
  • What sort of stuff should I be looking out for with regard to "unexpected concequences". It looks like "correct_ans", "correct_ans_latex_string", "original_formula", "equation" won't be inherited anymore. Is the issue is that some things may have been making use of these inherited values?

@goehle
Copy link
Member

goehle commented Dec 14, 2015

The test cases all check out, although I'm still curious about the answer to the second question. I'm merging this in any case.

goehle added a commit that referenced this pull request Dec 14, 2015
Fix problem with correct_ans being incorrectly set
@goehle goehle merged commit 60c5635 into openwebwork:release-2.11 Dec 14, 2015
@dpvc
Copy link
Member Author

dpvc commented Dec 15, 2015

What sort of stuff should I be looking out for with regard to "unexpected concequences". It looks like "correct_ans", "correct_ans_latex_string", "original_formula", "equation" won't be inherited anymore. Is the issue is that some things may have been making use of these inherited values?

Yes, that is my concern. The make() method that is changed here is used by every MathObject class (unless it is overridden, as in the Complex and Interval objects), and so changing it means that everything is affected. That is always a source of concern, though I can't think of anything specific that will go wrong.

The correct_ans and correct_ans_latex_string fields are the ones that needed to be removed to get the answers to show up correctly, and these are not used anywhere internally for MathObjects, so that is certainly safe. The original_formula should be removed, so that is also OK. The equation field is one that gets used, so there is some chance that that is a problem, but it appears to be added in again later in the process, so that it points to the correct location, so that should not be a problem either.

Note, however, that the noInherit list can be modified by the various classes, so there may be other losses as well. For example, the Formula class adds a bunch of Formula-specific fields to the list; but these all look good to me, so I think it is all OK. It's jus that MathObjects is a big library that has lots of complicated interactions, so something unexpected could occur. I ran the test suite that I have for the core MathObject classes, and everything was fine, but it is not as comprehensive as I would like (it hasn't been updated in years).

In any case, I think everything should be OK but just wanted you to have it in mind in case something strange shows up in the future.

@dpvc dpvc deleted the patch-inherit branch December 15, 2015 11:36
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

2 participants