WIP: Substitute symbols for expressions from optional scope #212

Ephs05msm · 2017-07-27T00:30:00Z

This is some work based on a discussion in #209. This happens to be my first ever pull request, so please provide any feedback on things that seem out of sorts. I referenced the contributing guidelines and then followed the instructions from this video to get to this point.

With that said, the approach I ended up taking toward this issue was to:

Add an optional scope argument to solveEquationString and simplifyExpressionString
Add a module, Substitute.js, that iterates over a provided scope and replaces symbols with their respective expressions. The intent is to account for nested symbols and the unit test I added to solveEquation.test.js uses such a scenario. I placed the module in ./lib but it likely belongs somewhere else.
Add a ChangeType, SUBSTITUTE_SCOPE_SYMBOL, that captures instances where symbols are being substituted. I felt this was important to keep the user experience consistent with what it currently is.
Add a tree search type, substituteScopeSearch, which compares SymbolNodes against the scope. When a match is found, simplifyExpressionString is run (awkwardly due to a TypeError: "...not a function" issue I ran into -- please assist) against the substituted expression and the resulting list of steps (if any) are passed to Node.Status.nodeChanged.

All tests pass, although I only added one additional test to begin to cover this functionality.

I did my best to employ similar patterns to those I found in the codebase as much as I could. There are sure to be some clumsily-handled areas; please provide feedback on those as I am still learning.

Thanks and I look forward to your review,
Matt Marino

…earch

…eType of SUBSTITUTE_SCOPE_SYMBOL.

Ephs05msm · 2017-07-27T00:46:40Z

Among other mistakes, I accidentally created a test file early on and then never ended up using it. Should probably be removed. Sorry about that.

evykassirer · 2017-07-27T01:50:49Z

yeah feel free to go through and remove anything not directly related to your PR

I noticed there's also a yarn.lock file which shouldn't be in this PR either - you could add it to the gitignore file!

I think it's getting late for me tonight but I hope to take a look at this in the next couple days :) Thanks for your hard work (it's so cool seeing your learning process and how you do so much without much formal educational background!!!) and for your patience :)

Ephs05msm · 2017-07-27T11:46:29Z

Sounds good. Couple more commits to get rid of some of the obviously unnecessary stuff. Still a bit of a noob with .gitignore so struggled to get the .vscode folder out. Sorry about that.

Saw a conflicting note regarding yarn.lock so just left it alone for now.

Look forward to your feedback whenever you have time. No rush at all on my end. Thanks again!

Ephs05msm · 2017-07-27T19:25:20Z

Not directly related to this issue, but I wanted to ask a quick question:

While using this code on another project I came across the following scenario that confused me. I added it to the test suite on my local branch and got the same result:

Same result on master as well (using ['x = 100 + (25^2 / 4)', 'x = 256.25']).

Is it by design that MathSteps won't simplify fractions that could be expressed as a float but not an (unrounded) integer?

evykassirer · 2017-07-28T18:20:00Z

answered in #215 :)

Ephs05msm · 2017-08-04T16:25:30Z

Ran into a case where a symbol was nested inside of a UnaryMinus node type and adjusted to account for this. Also added a test to cover. My editor (VS Code) was having end of line sequence issues so fixing those makes it look like I changed a lot of things when in fact only the tests, TreeSearch, and scopeSubstitution search type were meaningfully changed. Sorry about the extra noise. All precommit checks pass.

evykassirer

Hi! Thanks for your patience in waiting for review :)

This looks super awesome! I am very impressed that, with no formal background in CS, you were able to just jump in and make this big change and follow the style of code already here and wow 🎉

I added a bunch of comments (I always do in pull requests hehe) mostly small things, but a few bigger changes. No opinions are absolute so let me know if you think something else would be a better idea or if you have questions about what I mean

Also a high level comment:

I think instead of passing in scope, we should pass in options.

right now it'd look like:

options = {
  scope: { ... }
}

Then later if we want to something else alongside scope, we don't have to add an extra argument everywhere, we can just add it to the dictionary. How do you feel about that?

evykassirer · 2017-07-31T23:24:28Z

test/solveEquation/solveEquation.test.js

@@ -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)


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)

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

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

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.

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!)

evykassirer · 2017-08-06T00:19:06Z

.gitignore

+node_modules
+*.log
+.DS_Store
+.idea


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!

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

evykassirer · 2017-08-06T00:27:36Z

lib/TreeSearch.js

    if (status.hasChanged()) {
      return Node.Status.childChanged(node, status);
    }
  }
+  // Break out isSymbol test and add a changeType for SUBSTITUTE_SYMBOL?


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

evykassirer · 2017-08-06T00:29:01Z

lib/TreeSearch.js

    return Node.Status.noChange(node);
  }
+  // Moved isUnaryMinus check above Symbol check to address symbol nested inside unaryminus


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 ^^)

evykassirer · 2017-08-06T00:31:53Z

lib/Substitute.js

@@ -0,0 +1,16 @@
+function substituteScope(scope) {


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

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?

evykassirer · 2017-08-06T00:33:36Z

lib/simplifyExpression/stepThrough.js

@@ -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.


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

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

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

evykassirer · 2017-08-06T00:33:49Z

lib/simplifyExpression/substituteScopeSearch/index.js

+
+// 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.


awesome documentation :D

evykassirer · 2017-08-06T00:40:50Z

lib/simplifyExpression/substituteScopeSearch/index.js

+  }
+
+  let symbolName;
+  if (node.type === 'SymbolNode') {


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

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)

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

evykassirer · 2017-08-06T00:44:53Z

lib/simplifyExpression/substituteScopeSearch/index.js

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


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?

evykassirer · 2017-08-06T00:49:52Z

lib/simplifyExpression/substituteScopeSearch/index.js

+  if (scope.hasOwnProperty(symbolName)) {
+    // when declared at top, kept getting TypeError: ___ is not a function
+    const simplifyExpression = require('../../simplifyExpression');
+    const substeps = simplifyExpression(scope[symbolName]);


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?

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.

evykassirer · 2017-08-06T04:08:06Z

ok so I looked into it and there's no reason for allowUnaryMinus to default to true - I've updated that on master, feel free to pull that in and then your code should work as it was before :)

Ephs05msm · 2017-08-07T14:29:27Z

Thanks so much for the detailed feedback! I'm all for your suggestions and will get to work on them this week. If I get in over my head anywhere I'll ask for help.

Also, I really appreciate the encouragement. I've found there's a lot of self-doubt along the learning path (at least doing it the way I have been), so even a little bit of affirmation that some things are being done correctly is hugely helpful. Thank you.

On we go :-)

…ptions, add documentation, begin breaking out substituteScope tests

Ephs05msm · 2017-08-10T22:17:33Z

I replaced the scope arg with options as you suggested, and also attempted to reorder the args so debug is last. Tests pass but it's certainly possible I didn't address it in every place/case. Happy to double back with you if you find issues.

Ephs05msm · 2017-08-10T22:19:38Z

Thanks again for taking the time to review. I think this latest round of changes get closer to the mark, but there are still some loose ends (especially on the testing side). I felt it better to commit some progress than hold back just to spin my wheels in a few spots. Look forward to continuing to work with you on this :-).

evykassirer · 2017-08-30T04:49:59Z

lib/substituteScope.js

@@ -0,0 +1,24 @@
+function substituteScope(scope) {


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?

evykassirer · 2017-08-30T04:53:07Z

lib/simplifyExpression/substituteScopeSearch/index.js

  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


^ still gotta figure this thing out

I will look into this...

evykassirer · 2017-08-30T04:57:25Z

lib/TreeSearch.js

    return Node.Status.noChange(node);
  }
+  // 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.


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

evykassirer · 2017-08-30T04:59:43Z

lib/simplifyExpression/substituteScopeSearch/index.js

+  if (node.type === 'SymbolNode') {
+    symbolName = node.name;
+  }
+  else if (Node.Type.isUnaryMinus(node)) {


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;

evykassirer · 2017-08-30T05:00:33Z

lib/simplifyExpression/substituteScopeSearch/index.js

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


what is nameIGaveToFunction - which function?

...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.

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!

evykassirer · 2017-08-30T05:12:32Z

lib/simplifyExpression/substituteScopeSearch/index.js

+  if (scope.hasOwnProperty(symbolName)) {
+    // when declared at top, kept getting
+    // TypeError: <nameIGaveToFunction> is not a function
+    const simplifyExpression = require('../../simplifyExpression');


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:

take out the const newScope = Substitute(options.scope); above and keep the options exactly as they were passed in

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!

evykassirer · 2017-08-30T05:13:46Z

ok I think I addressed all your remaining comments/questions!

looks really awesome so far, I'm a fan of the options thing and debug last is 👌 (I am also not sure if you caught them all but seems good!) Thanks for your patience with my review and for working on this new really interesting feature!!

Ephs05msm · 2017-09-02T17:07:58Z

Thanks, @evykassirer! Planning to spend time on this next week.

Ephs05msm · 2017-09-27T17:44:22Z

Hi @evykassirer, just wanted to to circle back and let you know that I haven't lost sight of this. Just been focused on a few other things instead. Since this is an isolated branch representing a marginal feature, is it safe to assume that it's alright if I take a break for a bit? If not, I'll make the time.

Thanks again for your help.

evykassirer · 2017-09-29T03:05:18Z

no worries! I think you're fine, but if the porting change goes through soon and messes you up I'd be happy to help you refactor your stuff (it'd be just a bunch of really small changes) :)

Ephs05msm added 6 commits July 25, 2017 16:43

Allow for optional scope arg in functions leading to substituteScopeS…

b1fa321

…earch

Failed implementation attempt

9ed6e86

First pass at pseudo-code for substitution

9e225bd

SolveEquation and SimplifyExpression accept optional scope. New Chang…

c62a82d

…eType of SUBSTITUTE_SCOPE_SYMBOL.

Add equation unit test for substitution of symbols

1219478

Remove leftover example file

633522f

Ephs05msm added 3 commits July 27, 2017 07:29

Revise gitignore, remove some unused code

6effc8b

Revised gitignore

b382470

Delete .vscode folder

de2fa8e

End of line sequence revisions

921585d

Merge branch 'master' into mm_use_scope to sync with upstream master

0cbfe98

evykassirer reviewed Aug 6, 2017

View reviewed changes

Ephs05msm added 2 commits August 10, 2017 15:16

Merge branch 'master' into mm_use_scope to stay in sync with upstream.

f378d87

Make debug last arg in simplify and solve, replace 'scope' arg with o…

c18ba6c

…ptions, add documentation, begin breaking out substituteScope tests

evykassirer reviewed Aug 30, 2017

View reviewed changes

WIP: Substitute symbols for expressions from optional scope #212

Are you sure you want to change the base?

WIP: Substitute symbols for expressions from optional scope #212

Conversation

Ephs05msm commented Jul 27, 2017

Ephs05msm commented Jul 27, 2017

evykassirer commented Jul 27, 2017

Ephs05msm commented Jul 27, 2017

Ephs05msm commented Jul 27, 2017 • edited by evykassirer Loading

evykassirer commented Jul 28, 2017

Ephs05msm commented Aug 4, 2017

evykassirer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evykassirer Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evykassirer Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evykassirer commented Aug 6, 2017

Ephs05msm commented Aug 7, 2017

Ephs05msm commented Aug 10, 2017

Ephs05msm commented Aug 10, 2017

evykassirer Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evykassirer commented Aug 30, 2017

Ephs05msm commented Sep 2, 2017

Ephs05msm commented Sep 27, 2017

evykassirer commented Sep 29, 2017

Ephs05msm commented Jul 27, 2017 •

edited by evykassirer

Loading

evykassirer Aug 6, 2017 •

edited

Loading

evykassirer Aug 6, 2017 •

edited

Loading

evykassirer Aug 30, 2017 •

edited

Loading