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
+1,148 −35
Open

WIP: Substitute symbols for expressions from optional scope #212

Changes from 1 commit
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
Prev Previous commit
Next Next commit
End of line sequence revisions
@Ephs05msm
Ephs05msm committed Aug 4, 2017
commit 921585d782a98c7034c00910da22fecc0f8669fa
8 changes: 4 additions & 4 deletions .gitignore
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
*swp
node_modules
*.log
.DS_Store
*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.

9 changes: 5 additions & 4 deletions lib/TreeSearch.js
View file
Original file line number Diff line number Diff line change
@@ -32,16 +32,17 @@ function search(simplificationFunction, node, preOrder, scope={}) {
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);
}
// 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, 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];
9 changes: 8 additions & 1 deletion lib/simplifyExpression/substituteScopeSearch/index.js
View file
Original file line number Diff line number Diff line change
@@ -23,7 +23,14 @@ function substituteAndSimplifySymbols(node, scope) {
return Node.Status.noChange(node);
}

const symbolName = node.name;
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!

15 changes: 14 additions & 1 deletion test/solveEquation/solveEquation.test.js
View file
Original file line number Diff line number Diff line change
@@ -97,7 +97,20 @@ describe('solveEquation for =', function () {
['(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
Contributor

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
Contributor

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
Contributor

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
Author

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
Contributor

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 }]
['2y = baz - x^2', 'y = 400', false, {baz: '(bar^2)', x: 10, bar: '(foo + x)', foo: 20 }],
// Symbol nested inside UnaryMinus node type
['y = GrossVal * (1 - (FeePct + CCPct)) - TransferFee', 'y = 11471675.7525', false,
{
CCPct: 0.015,
FeePct: 0.05,
GrossVal: 'TotalSales * Margin * Multiple',
Margin: 0.15,
Multiple: 10,
PreTaxNetVal: 'GrossVal * (1 - (FeePct + CCPct)) - TransferFee',
TotalSales: 8239341,
TransferFee: 84000,
}
]
// 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'],
1,902 changes: 951 additions & 951 deletions yarn.lock
View file

Large diffs are not rendered by default.