Skip to content
This repository was archived by the owner on Aug 29, 2024. It is now read-only.
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
node_modules
*.log
.DS_Store
.idea
.idea
16 changes: 0 additions & 16 deletions lib/Substitute.js

This file was deleted.

28 changes: 15 additions & 13 deletions lib/TreeSearch.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,25 @@ 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, scope={}) {
return search(simplificationFunction, node, true, scope);
return function (node, options={}) {
return search(simplificationFunction, node, true, options);
};
};

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

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

if (preOrder) {
status = simplificationFunction(node, scope);
status = simplificationFunction(node, options);
if (status.hasChanged()) {
return status;
}
Expand All @@ -32,28 +32,30 @@ function search(simplificationFunction, node, preOrder, scope={}) {
if (Node.Type.isConstant(node)) {
return Node.Status.noChange(node);
}
// Moved isUnaryMinus check above Symbol check to address symbol nested inside unaryminus
// Check isUnaryMinus before Symbol because symbols nested inside unaryMinus
// (e.g., 2x - symbol) need to be subtracted from the expression. Checking
// symbol first causes the symbol to be added.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think with my unary minus change, it doesn't have to be checked first - can you switch it back and see if that works? Before, symbols with unary minuses were counted as symbols too by default, but now it Node.Type.isSymbol(node) on a unary minus node should return false by default :D

else if (Node.Type.isUnaryMinus(node)) {
status = search(simplificationFunction, node.args[0], preOrder, scope);
status = search(simplificationFunction, node.args[0], preOrder, options);
if (status.hasChanged()) {
return Node.Status.childChanged(node, status);
}
}
// Break out isSymbol test and add a changeType for SUBSTITUTE_SYMBOL?
// symbols can be simplified through variable substitution
else if (Node.Type.isSymbol(node)) {
return simplificationFunction(node, scope);
return simplificationFunction(node, options);
}
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, scope);
const childNodeStatus = search(simplificationFunction, child, preOrder, options);
if (childNodeStatus.hasChanged()) {
return Node.Status.childChanged(node, childNodeStatus, i);
}
}
}
else if (Node.Type.isParenthesis(node)) {
status = search(simplificationFunction, node.content, preOrder, scope);
status = search(simplificationFunction, node.content, preOrder, options);
if (status.hasChanged()) {
return Node.Status.childChanged(node, status);
}
Expand All @@ -63,7 +65,7 @@ function search(simplificationFunction, node, preOrder, scope={}) {
}

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

function simplifyExpressionString(expressionString, options={}, debug=false) {
const newOptions = Object.assign({}, options);
const newScope = Substitute(options.scope);
newOptions.scope = newScope;

function simplifyExpressionString(expressionString, debug=false, scope={}) {
const newScope = Substitute(scope);
let exprNode;
try {
exprNode = math.parse(expressionString);
Expand All @@ -12,7 +15,7 @@ function simplifyExpressionString(expressionString, debug=false, scope={}) {
return [];
}
if (exprNode) {
return stepThrough(exprNode, debug, newScope);
return stepThrough(exprNode, newOptions, debug);
}
return [];
}
Expand Down
11 changes: 6 additions & 5 deletions lib/simplifyExpression/stepThrough.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,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, scope={}) {
function stepThrough(node, options={}, debug=false) {
if (debug) {
// eslint-disable-next-line
console.log('\n\nSimplifying: ' + print.ascii(node, false, true));
Expand All @@ -38,15 +38,15 @@ function stepThrough(node, debug=false, scope={}) {
let iters = 0;

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

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

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

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

node = flattenOperands(node);
Expand All @@ -72,6 +72,7 @@ function step(node, scope={}) {
basicsSearch,
// Substitute symbols present in the optional scope object with
// their respective expressions.
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

// e.g., given options: { scope: { x: 10, y: 20 } }, x + y => 10 + 20
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
Expand All @@ -95,7 +96,7 @@ function step(node, scope={}) {
];

for (let i = 0; i < simplificationTreeSearches.length; i++) {
nodeStatus = simplificationTreeSearches[i](node, scope);
nodeStatus = simplificationTreeSearches[i](node, options);
// 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
16 changes: 11 additions & 5 deletions lib/simplifyExpression/substituteScopeSearch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ const TreeSearch = require('../../TreeSearch');
// Returns a Node.Status object.
Copy link
Copy Markdown
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) {
function scopeSubstitution(node, options={}) {
if (Node.Type.isSymbol(node)) {
return substituteAndSimplifySymbols(node, scope);
return substituteAndSimplifySymbols(node, options);
}
else {
return Node.Status.noChange(node);
Expand All @@ -18,7 +18,11 @@ function scopeSubstitution(node, scope) {

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

if (!Node.Type.isSymbol(node)) {
return Node.Status.noChange(node);
}
Expand All @@ -31,8 +35,10 @@ function substituteAndSimplifySymbols(node, scope) {
symbolName = node.args[0].name;
}

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

Choose a reason for hiding this comment

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

^ still gotta figure this thing out

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will look into this...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what is nameIGaveToFunction - which function?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

...this was my way of trying to communicate that, no matter what name I gave to the function that made this call, it would fail with that error message. nameIGaveToFunction would be substituted with whatever I tried. IIRC, I pointed this out because I thought the issue might have been with a name conflict of some kind. Doesn't appear to be the case, though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

which is the function making the call? is it in the PR here? what's it called right now?

There's so many functions in the code haha so confused which one you're referring to!

const simplifyExpression = require('../../simplifyExpression');
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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) {
Expand All @@ -51,4 +57,4 @@ function substituteAndSimplifySymbols(node, scope) {
}
}

module.exports = search;
module.exports = search;
12 changes: 8 additions & 4 deletions lib/solveEquation/index.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
const math = require('mathjs');
const stepThrough = require('./stepThrough');
const Substitute = require('../Substitute');
const Substitute = require('../substituteScope');

function solveEquationString(equationString, options={}, debug=false) {
const newOptions = Object.assign({}, options);
const newScope = Substitute(options.scope);
newOptions.scope = newScope;

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

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

Expand Down
20 changes: 10 additions & 10 deletions lib/solveEquation/stepThrough.js
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, scope={}) {
let equation = new Equation(leftNode, rightNode, comparator, scope);
function stepThrough(leftNode, rightNode, comparator, options={}, debug=false) {
let equation = new Equation(leftNode, rightNode, comparator);

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

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

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

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

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

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

// 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=[], scope={}) {
function solveConstantEquation(equation, debug, steps=[], options={}) {
const compareFunction = COMPARATOR_TO_FUNCTION[equation.comparator];

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

steps = addSimplificationSteps(steps, equation, debug, scope);
steps = addSimplificationSteps(steps, equation, options, debug);
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, scope={}) {
function addSimplificationSteps(steps, equation, options={}, debug=false) {
let oldEquation = equation.clone();

/*
Expand All @@ -321,7 +321,7 @@ function addSimplificationSteps(steps, equation, debug=false, scope={}) {
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, scope);
const leftSimplifySteps = simplifyExpressionNode(equation.leftNode, options, false);
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, scope={}) {
// (ie the "before" of the before and after) for simplifying the right side.
oldEquation = equation.clone();

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

for (let i = 0; i < rightSteps.length; i++) {
Expand Down
24 changes: 24 additions & 0 deletions lib/substituteScope.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
function substituteScope(scope) {
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.

just copying from before so it's easier to keep track of what's left (no worries for not getting to it yet)

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?

// create a copy of the provided scope
const newScope = Object.assign({}, scope);

// iterate over symbols
for (var symbol in newScope) {
// store the symbol's current value in a variable
const targetVal = newScope[symbol].toString();
// iterate through scope (again)
for (var sym in newScope) {
// extract the value of a symbol as a string
const valStr = newScope[sym].toString();
// within the stored value, replace symbol (from the outer loop)
// with its respective value
const replaced = valStr.replace(symbol, targetVal);
// store the substituted value in the new scope under the same symbol
newScope[sym] = replaced;
}
}

return newScope;
}

module.exports = substituteScope;
Loading