maths / moodle-qtype_stack

Stack question type for Moodle
GNU General Public License v3.0
142 stars 149 forks source link

ev() expands its scope for argument simp #1213

Open Mat-Ge opened 4 months ago

Mat-Ge commented 4 months ago

The last ev() in an expression seems to impose its simplification property on the entire expression. Since the numer argument does not do the same, this seems to be rather a simp than an ev() issue. I use STACK 4.6 and can't reproduce this error in the Maxima sandbox. grafik

sangwinc commented 4 months ago

Could you post the test-case text into the issue please so I don't have to re-type it?! Thanks.

Mat-Ge commented 4 months ago

Sure, here it is:

sangwinc commented 4 months ago

Is the problem that in the last example you have abs(1/2)*a is evaluated to a/2 and not left as |1/2|*a?

Here is a minimal example, and the last line has a workaround

{@abs(1/2)*a+abs(2/5)*b@}<br/>
{@abs(1/2)*a+ev(abs(2/5)*b,simp)@} <br/>
{@abs(1/2)*a+abs(2/5)*b@}<br/>
{@(simp:false,abs(1/2)*a+abs(2/5)*b)@}

I think this is to do with the way castext is extracted and evaluated. The alternative is to use variables, as follows.

p1:abs(1/2)*a+abs(2/5)*b;
p2:abs(1/2)*a+ev(abs(2/5)*b,simp);
p3:abs(1/2)*a+abs(2/5)*b;
p4:(simp:false,abs(1/2)*a+abs(2/5)*b);

and then the catext

{@p1@}<br/>
{@p2@}<br/>
{@p3@}<br/>
{@p4@}
Mat-Ge commented 4 months ago

An introductory simp:false is a workaround, thanks for this idea. I mentioned the problem in an expression like {@score-penalty=ev(score-penalty,simp)@}. In my opinion ev(exp, simp) does currently not working as intended in CASText. The last line with ev(exp, numer) in my first example was just to check whether the last ev() imposes the property of all arguments on the entire expression. grafik

aharjula commented 4 months ago

The injection of values to CASText is a tricky mess, and it currently does not support mixed simplification settings within a singular injection. The fact that it reacts the way it does when it spots any logic inside of it affecting simp is not an easy thing and has some serious historical baggage in the form of unit tests. Furthermore, those injections are supposed to be separate evaluation contexts, which makes handling local overrides of simp somewhat complicated when we don't want changes to the value leaking.

Do note that the current logic does not even try to identify mixed simp. It just ensures that if any part of the expression touches simp, then the whole is assumed to be of that same setting; otherwise, it is always using the context simp. Anyone willing to make it deal with that without changing current unit test behaviour is free to try to do that but I doubt that is worth it.

Note that anyone can try to change way the contents of {@...@} are turned into an expression that returns a rendered string by editing the matching block, but I doubt there is a sensible way for keeping subtrees of an expression with different simplification settings active intact through logic that needs to turn those into LaTeX. Other than the basic, construct elsewhere and inject with simp:false.

The preferred way for this is the variable-based way Chris suggested, but you always use the [[define/]] block to construct those variables within the CASText if need be.

[[comment]]
Should produce 1+2 no matter what the global simp setting is. 
Will also leave the CASText level simp to true, which is  a bit bad form if it was originally false
[[/comment]]
[[define simp="false" _tmp="1+ev(1+1,simp)" simp="true"/]]
{@_tmp,simp=false@}
sangwinc commented 4 months ago

Since this is going to be very complex to fix, and there is a workaround, I'm going to park this issue in the "backlog" for now. Thanks for reporting it.