Skip to content
This repository was archived by the owner on Aug 29, 2024. It is now read-only.

WIP: Substitute symbols for expressions from optional scope #212

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

WIP: Substitute symbols for expressions from optional scope #212

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b1fa321
Allow for optional scope arg in functions leading to substituteScopeS…
Ephs05msm Jul 25, 2017
9ed6e86
Failed implementation attempt
Ephs05msm Jul 26, 2017
9e225bd
First pass at pseudo-code for substitution
Ephs05msm Jul 26, 2017
c62a82d
SolveEquation and SimplifyExpression accept optional scope. New Chang…
Ephs05msm Jul 26, 2017
1219478
Add equation unit test for substitution of symbols
Ephs05msm Jul 26, 2017
633522f
Remove leftover example file
Ephs05msm Jul 26, 2017
6effc8b
Revise gitignore, remove some unused code
Ephs05msm Jul 27, 2017
b382470
Revised gitignore
Ephs05msm Jul 27, 2017
de2fa8e
Delete .vscode folder
Ephs05msm Jul 27, 2017
921585d
End of line sequence revisions
Ephs05msm Aug 4, 2017
0cbfe98
Merge branch 'master' into mm_use_scope to sync with upstream master
Ephs05msm Aug 4, 2017
f378d87
Merge branch 'master' into mm_use_scope to stay in sync with upstream.
Ephs05msm Aug 10, 2017
c18ba6c
Make debug last arg in simplify and solve, replace 'scope' arg with o…
Ephs05msm Aug 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
*swp
node_modules
*.log
.DS_Store
.idea
*swp
node_modules
*.log
.DS_Store
.idea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you keep the new line at the end of the file? Github seems to like it and puts a 🚫 if you don't, so I've adopted liking it too ^_^

I see it in a few other files, so just scroll this PR and you'll see where!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully caught all these in my latest PR. Will pay attention to this going forward.

5 changes: 5 additions & 0 deletions lib/ChangeTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
module.exports = {
NO_CHANGE: 'NO_CHANGE',

// SUBSTITUTION

// e.g., given scope = { foo: 10 }, foo ^ 2 -> 10 ^ 2
SUBSTITUTE_SCOPE_SYMBOL: 'SUBSTITUTE_SCOPE_SYMBOL',

// ARITHMETIC

// e.g. 2 + 2 -> 4 or 2 * 2 -> 4
Expand Down
16 changes: 16 additions & 0 deletions lib/Substitute.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
function substituteScope(scope) {
Copy link
Contributor

@evykassirer evykassirer Aug 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you name this file substituteScope.js to match its name? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also can you add a comment explaining the purpose of this function? I'm a bit confused about what it does - is it replacing scope things? Shouldn't that happen in the steps instead?

const newScope = Object.assign({}, scope);

for (var symbol in newScope) {
const targetVal = newScope[symbol].toString();
for (var sym in newScope) {
const valStr = newScope[sym].toString();
const replaced = valStr.replace(symbol, targetVal);
newScope[sym] = replaced;
}
}

return newScope;
}

module.exports = substituteScope;
27 changes: 16 additions & 11 deletions lib/TreeSearch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,50 +5,55 @@ const TreeSearch = {};
// Returns a function that performs a preorder search on the tree for the given
// simplification function
TreeSearch.preOrder = function(simplificationFunction) {
return function (node) {
return search(simplificationFunction, node, true);
return function (node, scope={}) {
return search(simplificationFunction, node, true, scope);
};
};

// Returns a function that performs a postorder search on the tree for the given
// simplification function
TreeSearch.postOrder = function(simplificationFunction) {
return function (node) {
return search(simplificationFunction, node, false);
return function (node, scope={}) {
return search(simplificationFunction, node, false, scope);
};
};

// A helper function for performing a tree search with a function
function search(simplificationFunction, node, preOrder) {
function search(simplificationFunction, node, preOrder, scope={}) {
let status;

if (preOrder) {
status = simplificationFunction(node);
status = simplificationFunction(node, scope);
if (status.hasChanged()) {
return status;
}
}

if (Node.Type.isConstant(node) || Node.Type.isSymbol(node)) {
if (Node.Type.isConstant(node)) {
return Node.Status.noChange(node);
}
// Moved isUnaryMinus check above Symbol check to address symbol nested inside unaryminus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to avoid saying things like "moved" since future people looking at this code will only know the current state, not the past and that'd be confusing!

you could instead say something like "isUnaryMinus needs to be checked before Symbol because ..."

Though I'm confused why it has to be first. What test would fail if you checked symbol first? A unary minus is not a symbol, and unary minus recurses on its child! (let me know if I'm being confusing, not sure if I'm explaining this well ^^)

else if (Node.Type.isUnaryMinus(node)) {
status = search(simplificationFunction, node.args[0], preOrder);
status = search(simplificationFunction, node.args[0], preOrder, scope);
if (status.hasChanged()) {
return Node.Status.childChanged(node, status);
}
}
// Break out isSymbol test and add a changeType for SUBSTITUTE_SYMBOL?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need this comment (it's more like a commit message and less like a long term comment, like above)

Though if you want to keep a comment here it'd probably be more like // symbols can be simplified through variable substitution

else if (Node.Type.isSymbol(node)) {
return simplificationFunction(node, scope);
}
else if (Node.Type.isOperator(node) || Node.Type.isFunction(node)) {
for (let i = 0; i < node.args.length; i++) {
const child = node.args[i];
const childNodeStatus = search(simplificationFunction, child, preOrder);
const childNodeStatus = search(simplificationFunction, child, preOrder, scope);
if (childNodeStatus.hasChanged()) {
return Node.Status.childChanged(node, childNodeStatus, i);
}
}
}
else if (Node.Type.isParenthesis(node)) {
status = search(simplificationFunction, node.content, preOrder);
status = search(simplificationFunction, node.content, preOrder, scope);
if (status.hasChanged()) {
return Node.Status.childChanged(node, status);
}
Expand All @@ -58,7 +63,7 @@ function search(simplificationFunction, node, preOrder) {
}

if (!preOrder) {
return simplificationFunction(node);
return simplificationFunction(node, scope);
}
else {
return Node.Status.noChange(node);
Expand Down
6 changes: 4 additions & 2 deletions lib/simplifyExpression/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
const math = require('mathjs');
const stepThrough = require('./stepThrough');
const Substitute = require('../Substitute');

function simplifyExpressionString(expressionString, debug=false) {
function simplifyExpressionString(expressionString, debug=false, scope={}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice if debug was the last argument, though I know that's a lot of moving code around so let me know if you want help going through and editing it all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I think we should replace scope with options - I'll say more about this in a top level comment on this PR

const newScope = Substitute(scope);
let exprNode;
try {
exprNode = math.parse(expressionString);
Expand All @@ -10,7 +12,7 @@ function simplifyExpressionString(expressionString, debug=false) {
return [];
}
if (exprNode) {
return stepThrough(exprNode, debug);
return stepThrough(exprNode, debug, newScope);
}
return [];
}
Expand Down
14 changes: 9 additions & 5 deletions lib/simplifyExpression/stepThrough.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const divisionSearch = require('./divisionSearch');
const fractionsSearch = require('./fractionsSearch');
const functionsSearch = require('./functionsSearch');
const multiplyFractionsSearch = require('./multiplyFractionsSearch');
const substituteScopeSearch = require('./substituteScopeSearch');

const clone = require('../util/clone');
const flattenOperands = require('../util/flattenOperands');
Expand All @@ -19,7 +20,7 @@ const removeUnnecessaryParens = require('../util/removeUnnecessaryParens');

// Given a mathjs expression node, steps through simplifying the expression.
// Returns a list of details about each step.
function stepThrough(node, debug=false) {
function stepThrough(node, debug=false, scope={}) {
if (debug) {
// eslint-disable-next-line
console.log('\n\nSimplifying: ' + print.ascii(node, false, true));
Expand All @@ -37,15 +38,15 @@ function stepThrough(node, debug=false) {
let iters = 0;

// Now, step through the math expression until nothing changes
nodeStatus = step(node);
nodeStatus = step(node, scope);
while (nodeStatus.hasChanged()) {
if (debug) {
logSteps(nodeStatus);
}
steps.push(removeUnnecessaryParensInStep(nodeStatus));

node = Status.resetChangeGroups(nodeStatus.newNode);
nodeStatus = step(node);
nodeStatus = step(node, scope);

if (iters++ === MAX_STEP_COUNT) {
// eslint-disable-next-line
Expand All @@ -60,7 +61,7 @@ function stepThrough(node, debug=false) {

// Given a mathjs expression node, performs a single step to simplify the
// expression. Returns a Node.Status object.
function step(node) {
function step(node, scope={}) {
let nodeStatus;

node = flattenOperands(node);
Expand All @@ -69,6 +70,9 @@ function step(node) {
const simplificationTreeSearches = [
// Basic simplifications that we always try first e.g. (...)^0 => 1
basicsSearch,
// Substitute symbols present in the optional scope object with
// their respective expressions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add an example similar to the other simplification searches here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an example but let me know if it needs to be reworded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me! awesome
I think you can take out the comma after the e.g. though

substituteScopeSearch,
// Simplify any division chains so there's at most one division operation.
// e.g. 2/x/6 -> 2/(x*6) e.g. 2/(x/6) => 2 * 6/x
divisionSearch,
Expand All @@ -91,7 +95,7 @@ function step(node) {
];

for (let i = 0; i < simplificationTreeSearches.length; i++) {
nodeStatus = simplificationTreeSearches[i](node);
nodeStatus = simplificationTreeSearches[i](node, scope);
// Always update node, since there might be changes that didn't count as
// a step. Remove unnecessary parens, in case one a step results in more
// parens than needed.
Expand Down
54 changes: 54 additions & 0 deletions lib/simplifyExpression/substituteScopeSearch/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
const ChangeTypes = require('../../ChangeTypes');
const Node = require('../../node');
const TreeSearch = require('../../TreeSearch');

// Searches through the tree, prioritizing deeper nodes, and substitutes
// in-scope values for their respective expressions on a symbol node if possible.
// Returns a Node.Status object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome documentation :D

const search = TreeSearch.postOrder(scopeSubstitution);

function scopeSubstitution(node, scope) {
if (Node.Type.isSymbol(node)) {
return substituteAndSimplifySymbols(node, scope);
}
else {
return Node.Status.noChange(node);
}
}

// SUBSTITUTES
// Returns a Node.Status object with substeps
function substituteAndSimplifySymbols(node, scope) {
if (!Node.Type.isSymbol(node)) {
return Node.Status.noChange(node);
}

let symbolName;
if (node.type === 'SymbolNode') {
Copy link
Contributor

@evykassirer evykassirer Aug 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use Node.Type.isSymbol(node) instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WAIT does this return true for unary minus too?

okay this makes sense https://github.com/socraticorg/mathsteps/blob/344d15b5d5e25251cef7d6976d6a73df16661d7d/lib/node/Type.js#L32

For any isSymbol thing you've added in this PR can you pass in false for the allowUnaryMinus argument? Then it can naturally recurse instead of you having to deal with it here.

Sorry, that's weird that we did that (though I'm assuming there's a reason I've now forgotten XD but feel free to investigate if you're curious or open an issue)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this alone for now since you followed up with the change to allowUnaryMinus. Let me know if we should revisit.

symbolName = node.name;
}
else if (Node.Type.isUnaryMinus(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now that I updated the Node.Type.isSymbol definition, you'll never get unary minuses in here anymore which makes things much easier hopefully!

so you can take out if (node.type === 'SymbolNode') { and else if (Node.Type.isUnaryMinus(node)) { and always just do symbolName = node.name;

symbolName = node.args[0].name;
}

if (scope.hasOwnProperty(symbolName)) {
// when declared at top, kept getting TypeError: ___ is not a function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain a bit more about when the error happens? I can try replicating it on my computer and see what's up. Did it say ___ is not a function or did it give a specific function name there?

const simplifyExpression = require('../../simplifyExpression');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so simplifyExpression is a pretty high level function - it's the big thing at the top that calls everything else. Because of that I feel weird about using it here.

What you have here also means if you have x = 2/3 + 4/6, the substitution will go from x to 8/6 with the adding of fractions as substeps, which from a teaching perspective also feels weird to me.

My suggestion instead is:

  1. take out the const newScope = Substitute(options.scope); above and keep the options exactly as they were passed in
  2. here, instead of doing const substeps = simplifyExpression(scope[symbolName]); do newNode = math.parse(scope[symbolName])

I know it's a bit different from your idea, so we could chat more about this and make sure we're on the same page and both happy with whatever we decide to do. I think adding some tests that show multiple steps will also make it more clear what this changes. Let me know if you have more questions or comments about this!

const substeps = simplifyExpression(scope[symbolName]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of simplifying the expression here, I think it's best to just do newNode = math.parse(scope[symbolName])

then any simplification of the value can happen in later steps instead of substeps and it makes this code less complicated!

what do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit turned around here, perhaps due to the approach I took. Since simplifyExpression is where the scope substitution takes place, I'm concerned calling math.parse(scope[symbolName]) here won't be effective since it will not perform the scope substitution. That may be why you advised to put the substituteScope call deeper in the simplify function?

Left as is in my recent (incremental) PR.

if (substeps.length === 0) {
const newNode = Node.Creator.constant(Number(scope[symbolName]));
return Node.Status.nodeChanged(
ChangeTypes.SUBSTITUTE_SCOPE_SYMBOL, node, newNode);
}
else {
const newNode = substeps.slice(-1)[0].newNode;
return Node.Status.nodeChanged(
ChangeTypes.SUBSTITUTE_SCOPE_SYMBOL, node, newNode, false, substeps);
}
}
else {
return Node.Status.noChange(node);
}
}

module.exports = search;
7 changes: 4 additions & 3 deletions lib/solveEquation/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
const math = require('mathjs');

const stepThrough = require('./stepThrough');
const Substitute = require('../Substitute');

function solveEquationString(equationString, debug=false) {
function solveEquationString(equationString, debug=false, scope={}) {
const newScope = Substitute(scope);
const comparators = ['<=', '>=', '=', '<', '>'];

for (let i = 0; i < comparators.length; i++) {
Expand All @@ -27,7 +28,7 @@ function solveEquationString(equationString, debug=false) {
return [];
}
if (leftNode && rightNode) {
return stepThrough(leftNode, rightNode, comparator, debug);
return stepThrough(leftNode, rightNode, comparator, debug, newScope);
}
}

Expand Down
20 changes: 10 additions & 10 deletions lib/solveEquation/stepThrough.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ const COMPARATOR_TO_FUNCTION = {
// the solution. Possible solutions include:
// - solving for a variable (e.g. 'x=3' for '3x=4+5')
// - the result of comparing values (e.g. 'true' for 3 = 3, 'false' for 3 < 2)
function stepThrough(leftNode, rightNode, comparator, debug=false) {
let equation = new Equation(leftNode, rightNode, comparator);
function stepThrough(leftNode, rightNode, comparator, debug=false, scope={}) {
let equation = new Equation(leftNode, rightNode, comparator, scope);

if (debug) {
// eslint-disable-next-line
Expand All @@ -40,7 +40,7 @@ function stepThrough(leftNode, rightNode, comparator, debug=false) {
const symbolSet = Symbols.getSymbolsInEquation(equation);

if (symbolSet.size === 0) {
return solveConstantEquation(equation, debug);
return solveConstantEquation(equation, debug, [], scope);
}
const symbolName = symbolSet.values().next().value;

Expand Down Expand Up @@ -68,7 +68,7 @@ function stepThrough(leftNode, rightNode, comparator, debug=false) {

// Step through the math equation until nothing changes
do {
steps = addSimplificationSteps(steps, equation, debug);
steps = addSimplificationSteps(steps, equation, debug, scope);

if (steps.length > 0) {
const lastStep = steps[steps.length - 1];
Expand All @@ -81,7 +81,7 @@ function stepThrough(leftNode, rightNode, comparator, debug=false) {

// at this point, the symbols might have cancelled out.
if (Symbols.getSymbolsInEquation(equation).size === 0) {
return solveConstantEquation(equation, debug, steps);
return solveConstantEquation(equation, debug, steps, scope);
}

// The left side of the equation is either factored or simplified.
Expand Down Expand Up @@ -237,14 +237,14 @@ function getSolutionsAndSymbol (equation) {

// Given an equation of constants, will simplify both sides, returning
// the steps and the result of the equation e.g. 'True' or 'False'
function solveConstantEquation(equation, debug, steps=[]) {
function solveConstantEquation(equation, debug, steps=[], scope={}) {
const compareFunction = COMPARATOR_TO_FUNCTION[equation.comparator];

if (!compareFunction) {
throw Error('Unexpected comparator');
}

steps = addSimplificationSteps(steps, equation, true, debug);
steps = addSimplificationSteps(steps, equation, debug, scope);
if (steps.length > 0) {
const lastStep = steps[steps.length - 1];
equation = Equation.createEquationFromString(
Expand Down Expand Up @@ -307,7 +307,7 @@ function step(equation, symbolName) {
}

// Simplifies the equation and returns the simplification steps
function addSimplificationSteps(steps, equation, debug=false) {
function addSimplificationSteps(steps, equation, debug=false, scope={}) {
let oldEquation = equation.clone();

/*
Expand All @@ -321,7 +321,7 @@ function addSimplificationSteps(steps, equation, debug=false) {
e.g. x + 4 + 2 = 0 <- simplify the left side
e.g. 0 = x^2 + 3x + 2 -> x^2 + 3x + 2 = 0 <- swap to the left side
*/
const leftSimplifySteps = simplifyExpressionNode(equation.leftNode, false);
const leftSimplifySteps = simplifyExpressionNode(equation.leftNode, false, scope);
const simplifiedLeftNode = leftSimplifySteps.length !== 0
? leftSimplifySteps.slice(-1)[0].newNode
: equation.leftNode;
Expand Down Expand Up @@ -369,7 +369,7 @@ function addSimplificationSteps(steps, equation, debug=false) {
// (ie the "before" of the before and after) for simplifying the right side.
oldEquation = equation.clone();

const rightSteps = simplifyExpressionNode(equation.rightNode, false);
const rightSteps = simplifyExpressionNode(equation.rightNode, false, scope);
const rightSubSteps = [];

for (let i = 0; i < rightSteps.length; i++) {
Expand Down
Loading