-
-
Notifications
You must be signed in to change notification settings - Fork 77
only use parentheses (not brackets) for Formula strings #1277
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
base: PG-2.20
Are you sure you want to change the base?
Conversation
This is actually false. The student answer displayed in the response popup (what used to be the results table) shows the string version, and uses the alternating parentheses. So students see these all the time. The reason, as you surmised, is to make it easier for students (not problem authors, though it can help them, too), to see the nesting of expressions. I would hate to lose that because an expression might sometime be used in a TikZ plot. Instead, it would be possible to have the Here is a patch that does that. diff --git a/lib/Parser/Context/Default.pm b/lib/Parser/Context/Default.pm
index 1f3e4ea1..c182fdcd 100644
--- a/lib/Parser/Context/Default.pm
+++ b/lib/Parser/Context/Default.pm
@@ -434,6 +434,7 @@ $flags = {
reduceConstants => 1, # 1 = automatically combine constants
reduceConstantFunctions => 1, # 1 = compute function values of constants
showExtraParens => 1, # 1 = add useful parens, 2 = make things painfully unambiguous
+ stringifyNoBrackets => 0, # 1 = only use parentheses not brackets when stringifying
formatStudentAnswer => 'evaluated', # or 'parsed' or 'reduced'
allowMissingOperands => 0, # 1 is used by Typeset context
allowMissingFunctionInputs => 0, # 1 is used by Typeset context
diff --git a/lib/Parser/Item.pm b/lib/Parser/Item.pm
index 127ecf4e..bdb22ec7 100644
--- a/lib/Parser/Item.pm
+++ b/lib/Parser/Item.pm
@@ -118,7 +118,7 @@ sub isSetOfReals { (shift)->type =~ m/^(Interval|Union|Set)$/ }
sub addParens {
my $self = shift;
my $string = shift;
- if ($string =~ m/^[^\[]*\(/) { return '[' . $string . ']' }
+ return '[' . $string . ']' if $string =~ m/^[^\[]*\(/ && !$self->context->flag('stringifyNoBrackets');
return '(' . $string . ')';
}
diff --git a/lib/Value/Formula.pm b/lib/Value/Formula.pm
index 1ee2e8d0..33839a81 100644
--- a/lib/Value/Formula.pm
+++ b/lib/Value/Formula.pm
@@ -112,7 +112,14 @@ sub _dot {
$l->SUPER::_dot($r, $flag);
}
-sub pdot { '(' . (shift->stringify) . ')' }
+sub pdot {
+ my $self = shift;
+ my $nobrackets = $self->context->flag('stringifyNoBrackets');
+ $self->context->flags->set(stringifyNoBrackets => 1);
+ my $string = '(' . ($self->stringify) . ')';
+ $self->context->flags->set(stringifyNoBrackets => $nobrackets);
+ return $string;
+}
#
# Call the Parser::Function call function See what you think of that idea. I haven't tested with TikZ as I don't really know its language. |
I too use the string option of math objects to create both function strings for JavaScript and TikZ/PGFPlots to plot, and have a hack to replace the @Alex-Jordan could you also update |
One easy change would be to make Line 263 in 3d1d351
be my $func = $formula . ""; as that would trigger the new no-bracket code when stringifying If you want to call my $oldFlags = Value::contextSet($context, flag1 => value1, flag2 => value2);
# do stuff with the new values of flag1 and flag2
Value::contextSet($context, %$oldFlags); That avoids having to keep lots of individual values for the old flags around. I didn't use it in the |
I'm trying this, but I'm missing something about it. I have these changes in place. And in a problem, I have
Each of Also, how would you feel if this changed so that the default was |
@Alex-Jordan The way I think it is setup is if you have My use case would be okay to make it default, but I don't really show students or anyone string output, I use the tex preview instead. But wonder if it would be worth while making this setting |
Though looking around, it isn't obvious to me where to modify the code to disable this option when creating the string for the entered |
OK, I don't understand how it works but if I go so far as to make a PGlateximage plot, it works with @dpvc's suggestion. Even though things like I'll update this to that code, and look into the Plots change too. |
OK, I updated this with @dpvc's suggestions. |
The concatenation operator is only called when the value is inserted into an otherwise non-empty string. I.e., there has to be something other than It also gets called when the string concatenation is explicit, as in It would be harder to fix creation of student answers to explicitly use brackets, as those answer are created all over the place in different answer checkers. The concatenation operator seemed a reasonable place to disable the brackets, as that would not be used for student answers, in general, and would be used in something like the TikZ situation you gave above. But it might mean changing some places like the |
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.
Tested the new feature works as expected. Brackets are now shown with $formula->string
or "$formula"
, while $formula . ''
and "x$formula"
don't use brackets. Setting the context flag stringifyNoBrackets => 1
always disables brackets. Also tested Plots
works with this, which it does.
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.
This looks fine to me. I haven't tested extensively though.
The string method of a MathObject Formula alternates nested parens between parentheses and brackets. The presence of any brackets means the formula string cannot be used as the function for a pgfplots graph to plot. (See an old forum post.)
I can't think of a need for the brackets. Only a problem author will have reason to directly see the string method of a Formula, and while the brackets would help them more clearly see groupings, they can probably deal with just having all parentheses.
So this PR changes it so that it's all parentheses, no brackets. I considered making a context flag for this but at the moment I don't see the point. This may be controversial, and I am hoping @dpvc can weigh in.