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
Ephs05msm 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 9 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
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
node_modules
*.log
.DS_Store
.idea
.idea
Copy link
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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
Copy Markdown
Author

@Ephs05msm Ephs05msm Aug 10, 2017

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
Copy Markdown
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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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;
26 changes: 15 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,54 @@ 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);
}
// Break out isSymbol test and add a changeType for SUBSTITUTE_SYMBOL?
else if (Node.Type.isSymbol(node)) {
return simplificationFunction(node, scope);
}
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);
}
}
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 +62,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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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(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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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
Copy Markdown
Author

@Ephs05msm Ephs05msm Aug 10, 2017

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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 30, 2017

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
47 changes: 47 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,47 @@
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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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);
}

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

@evykassirer evykassirer Aug 6, 2017

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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 30, 2017

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
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

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
Copy Markdown
Author

@Ephs05msm Ephs05msm Aug 10, 2017

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
8 changes: 5 additions & 3 deletions test/solveEquation/solveEquation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ const solveEquation = require('../../lib/solveEquation');

const NO_STEPS = 'no-steps';

function testSolve(equationString, outputStr, debug=false) {
const steps = solveEquation(equationString, debug);
function testSolve(equationString, outputStr, debug=false, scope={}) {
const steps = solveEquation(equationString, debug, scope);
let lastStep;
if (steps.length === 0) {
lastStep = NO_STEPS;
Expand Down Expand Up @@ -96,6 +96,8 @@ describe('solveEquation for =', function () {
['2/(1 + 1 + 4x) = 1/3', 'x = 1'],
['(3 + x) / (x^2 + 3) = 1', 'x = [0, 1]'],
['6/x + 8/(2x) = 10', 'x = 1'],
// Use of nested scope (i.e., baz depends on bar)
Copy link
Copy Markdown
Contributor

@evykassirer evykassirer Jul 31, 2017

Choose a reason for hiding this comment

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

can you actually make a new test file just for testing with scope? :) and add a bunch more tests for more equations but also expressions (no equals sign)

I'd also reoreder the arguments in the new test file so that the scope comes after the input and before the output (makes a bit more sense when reading it)

Copy link
Copy Markdown
Contributor

@evykassirer evykassirer Jul 31, 2017

Choose a reason for hiding this comment

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

ALSO THIS IS REALLY COOL that you added this functionality!!

Copy link
Copy Markdown
Contributor

@evykassirer evykassirer Aug 6, 2017

Choose a reason for hiding this comment

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

oo also could you test substeps to show how things get sub'd in one at a time? there are some tests that already test substeps you can probably base off of but let me know if you want help setting that up

Copy link
Copy Markdown
Author

@Ephs05msm Ephs05msm Aug 10, 2017

Choose a reason for hiding this comment

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

Made some progress on this, but I struggled to get any of the simplify tests to pass. Decided to push my incremental changes in hopes we can work through this together. The simplify tests weren't originally designed with scope in mind, and my quick attempt to incorporate it fell short.

Copy link
Copy Markdown
Contributor

@evykassirer evykassirer Aug 30, 2017

Choose a reason for hiding this comment

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

The simplify tests weren't originally designed with scope in mind, and my quick attempt to incorporate it fell short.

Yeah I think putting the tests for this in their own completely different file would help - then we don't need to update the other tests to deal with scope and can do a different setup for these tests. Let me know if you want help going through that - I'm a lot more free this week and next than I have been recently and would love to help (sorry for just getting to this PR again now!)

['2y = baz - x^2', 'y = 400', false, {baz: '(bar^2)', x: 10, bar: '(foo + x)', foo: 20 }]
// TODO: fix these cases, fail because lack of factoring support, for complex #s,
// for taking the sqrt of both sides, etc
// ['(x + y) (y + 2) = 0', 'y = -y'],
Expand All @@ -111,7 +113,7 @@ describe('solveEquation for =', function () {
// this gives us 6.3995 when it should actually be 6.4 :(
// ['x - 3.4= ( x - 2.5)/( 1.3)', 'x = 6.4']
];
tests.forEach(t => testSolve(t[0], t[1], t[2]));
tests.forEach(t => testSolve(t[0], t[1], t[2], t[3]));
});

describe('solveEquation for non = comparators', function() {
Expand Down
Loading