Divide like terms powers #154
Conversation
… corresponding tests.
… corresponding tests.
…ivide_like_terms_powers
|
I think I'll leave this PR without looking at it until we merge the multiply one, cause it might affect how you do this one (though lemme know if you think it would be a good idea to look at both at the same time) |
|
No, you can leave this until the other One is looked at! This depends on some stuff in the other PR :) |
…lyLikeTermConstantNodes" instead to create the power node with exponent and base and also check if all the arguments are of this form and have the same base. Also added some more tests.
… base instead. Also fixed a bug where it couldnt simplify e.g '10^3 * 10 * 10' because of collectLikeTerms
… for changes of structure.
… thing as "multiplyLikeTerms" but returns exponent with "-" instead of "+"
…r functions to make other things look neater.
…ivide_like_terms_powers
|
you need to resolve conflicts to pull in the multiplication changes! |
# Conflicts: # .gitignore # lib/ChangeTypes.js # lib/checks/index.js # lib/simplifyExpression/collectAndCombineSearch/multiplyLikeTerms.js # test/simplifyExpression/collectAndCombineSearch/collectAndCombineSearch.test.js
.gitignore
Outdated
| @@ -2,3 +2,4 @@ | |||
| node_modules | |||
| *.log | |||
| .DS_Store | |||
| .idea | |||
There was a problem hiding this comment.
make sure all files have exactly one new line at the end or else you'll see the 🚫 like here
lib/ChangeTypes.js
Outdated
|
|
||
| // e.g. 10^2 * 10^3 -> 10^(3+2) | ||
| // e.g. 10^4 / 10^2 -> 10^(4-2) | ||
| COLLECT_CONSTANT_EXPONENTS: 'COLLECT_CONSTANT_EXPONENTS', |
There was a problem hiding this comment.
can you make a separate change group for each?
COLLECT_CONSTANT_EXPONENTS_MULT
and
COLLECT_CONSTANT_EXPONENTS_DIV
would be good I think
| // e.g. 2x * 3x -> (2 * 3)(x * x) | ||
| MULTIPLY_COEFFICIENTS: 'MULTIPLY_COEFFICIENTS', | ||
| // e.g. 2x * x -> 2x ^ 2 | ||
| MULTIPLY_POLYNOMIAL_TERMS: 'MULTIPLY_POLYNOMIAL_TERMS', | ||
| // e.g x^5 / x^3 -> x^2 | ||
| DIVIDE_POLYNOMIAL_TERMS: 'DIVIDE_POLYNOMIAL_TERMS', |
There was a problem hiding this comment.
oo nice does this change not happen already? I feel like it's already handled in the cancelling code - see if that already happens
(but maybe this change group is more descriptive than cancelling?)
There was a problem hiding this comment.
It is handled by cancelling but I think it is clearer with this change group. Guess it depends on the exercise
There was a problem hiding this comment.
there are more cases in cancelling like 3x^2 / x that aren't cancelled here, so maybe we should leave it all handled there. also means less extra code doing the same thing which is good
lib/TreeSearch.js
Outdated
| @@ -3,15 +3,15 @@ const Node = require('./node'); | |||
| const TreeSearch = {}; | |||
|
|
|||
| // Returns a function that performs a preorder search on the tree for the given | |||
| // simplifcation function | |||
| // simplification function | |||
There was a problem hiding this comment.
wait didn't you fix this in the other PR? or was there more typos :p
thanks, either way XD
| const Node = require('../node'); | ||
|
|
||
| // Returns true if node is a division of constant power nodes | ||
| // where you can combine their exponents, e.g. 10^4 / 10^2 can become 10^2 |
There was a problem hiding this comment.
can you use a different example where all the exponents are different?
| // Returns true if node is a division of constant power nodes | ||
| // where you can combine their exponents, e.g. 10^4 / 10^2 can become 10^2 | ||
| // The node can be on the form c^n or c, as long is c is the same for all | ||
| function canDivideLikeTermConstantNodes(node) { |
There was a problem hiding this comment.
if this function is the exact same as multiply but with just the node.op being different (which I think it is?) you should share that code and pass in the op to the function - much cleaner :D
|
|
||
| // Returns true if the nodes are symbolic terms with the same symbol and no | ||
| // coefficients. | ||
| function canDivideLikeTermPolynomialNodes(node) { |
There was a problem hiding this comment.
(same thing about code reuse)
| // MULTIPLYING CONSTANT POWERS | ||
| // e.g. 10^2 * 10^3 -> 10^(2+3) | ||
| // MULTIPLYING/DIVIDING CONSTANT POWERS | ||
|
|
| // MULTIPLYING/DIVIDING CONSTANT POWERS | ||
|
|
||
| // e.g. 10^2 * 10^3 -> 10^(3+2) | ||
| // e.g. 10^4 / 10^2 -> 10^(4-2) | ||
| COLLECT_CONSTANT_EXPONENTS: 'COLLECT_CONSTANT_EXPONENTS', |
There was a problem hiding this comment.
can you have two changeGroups?
COLLECT_CONSTANT_EXPONENTS_MULT and COLLECT_CONSTANT_EXPONENTS_DIV for example
There was a problem hiding this comment.
Surely! Will be more descriptive
| @@ -0,0 +1,178 @@ | |||
| const arithmeticSearch = require('../arithmeticSearch'); | |||
There was a problem hiding this comment.
how much of this code is copied from multiplying and how much is new? ideally very little code should be copied
There was a problem hiding this comment.
Pretty much everything everything. It should do the same thing as multiply but with another operator. Should I just pass the operator "-" to multiplyLikeTerms? Will propably be cleaner
There was a problem hiding this comment.
send '/' not '-' but yes, that's what I'm thinking. You'll want to rename the file though to be more descriptive of its new purpose
|
Hey @evykassirer / @sebastianalbert ! Wondering what the status on this is, now that the multiply one has been merged? |
|
looks like it's waiting on code review fixes :) |
|
Cool -- @sebastianalbert, think you'll be able to get those done in the next week or so? Would love the experience to be consistent between multiplication and division |
|
Oh sorry! I've been really busy with school and other projekts. But I can get it done next week! |
|
Not a problem at all! Next week (or even the one after, whatever) would be awesome, thanks! |
|
Sorry about the delay! But I will take a stab at these the coming days again. Been really busy with other stuff and school.. |
Same thing as the multiplying of power, but dividing. E.g
10^5/10^3 => 10^2by doing the calculation10^5/10^3 => 10^(5-3) => 10^2.