<fix> Normalise quotients to form a/b * c #23

JoshdRod · 2026-01-01T01:36:24Z

Quotients of form ac/b and a/b * c have differing expression trees (see attached image).
So do a/b and 1/b * a.
Essentially, this change adds a pass that converts all expressions of the form ac/b -> 1/b * a * c

JoshdRod · 2026-01-01T17:26:25Z

Weirdly, now getting no response when evaluating 2*1/3

Quotients of form ac/b and a/b * c have differing expression trees (see attached image). Essentially, this change adds a pass that converts ac/b -> a/b * c. FIXME: Current question can't be normalised?? May be to do with the 2nd TODO. TODO: Because this change essentially swaps the / and * nodes around, the * and / need to physically swap places in the list if the / is the root node. TODO: This may still not make 1/b * c == c/b. This needs to be looked at!

If / is root node of expression, the / and * need to swap. This is because the transformation ends up with the * being the logical root of the tree, and our tree traversal implementations rely on the root of the tree being at the front of the list.

Function checkDivisorIsProduct is renamed to checkDividendIsProduct, as that is what the function does. In the quotient a/b, a is the dividend, and b is the divisor.

A / node in a valid tree always has a right child, and we can assume that the input to this function is a valid tree. May need to add tree validation at some step though!

See e5c7de0

If / node wasn't the root node, then this pass would fail, because the created * node was not set as its parent's right node.

Copilot

Pull request overview

This PR implements normalization of division expressions to ensure consistent expression tree structures. The change adds two normalization passes that convert quotients into a standardized form: ac/b → 1/b * a * c and a/b → 1/b * a, enabling more reliable comparison of mathematically equivalent expressions.

Key changes:

Added two normalization passes to the normaliseTree function to standardize division operations
Implemented helper functions checkDividendIsProduct and checkDividendIsNot1 to identify expressions needing normalization
Temporarily disabled normaliseTree call at initialization (marked with TODO)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-05T18:22:30Z

pages/index/script.js

+		let multiplyNode = {
+			content: '*',
+			type: "operator",
+			leftNode: tree.indexOf(a),
+			rightNode: currentNodeIndex,
+			parent: currentNode.parent,
+			depth: -1
+		};
+
+		tree.push(multiplyNode);


When creating the multiplyNode, leftNode is set to tree.indexOf(a) which is the index before the multiplyNode is pushed to the tree. However, after tree.push(multiplyNode), if currentNodeIndex is 0, the code swaps positions of nodes in the tree array. This swap invalidates the leftNode index that was stored. The leftNode should be recalculated or set after all tree modifications are complete.

Copilot · 2026-01-05T18:22:30Z

pages/index/script.js

+	// Create a dictionary of depth:node indices
+	let layers = {};
+	let maxLayer = 0;
+	for (let i = 0; i < tree.length; i++)


The loop variable i is used in the for loop declaration, but inside the loop body, currentNodeIndex is being used to access tree elements. This appears to be a copy-paste error from the old code. The variable currentNodeIndex should be set to i at the start of the loop body, or the loop should be changed to iterate using currentNodeIndex instead of i.

Suggested change

for (let i = 0; i < tree.length; i++)

currentNodeIndex = rootNodeIndex;

while (currentNodeIndex != -1)

Copilot · 2026-01-05T18:22:30Z

pages/index/script.js

+// Checks whether current node is a /, and is dividend is something other than 1
+// Essentially, checks for divisions in the form of a/b (as these need to be normalised to 1/b * a)
+// INPUTS: tree, node in tree
+// RETURNS: bool true is dividend is not 1, false if it is


The comment has a typo: "true is dividend" should be "true if dividend".

Suggested change

// RETURNS: bool true is dividend is not 1, false if it is

// RETURNS: bool true if dividend is not 1, false if it is

Copilot · 2026-01-05T18:22:31Z

pages/index/script.js

+		if (currentNodeIndex == 0)
+		{
+			let temp = multiplyNode;
+			tree[tree.indexOf(multiplyNode)] = currentNode;
+			tree[0] = temp;
+
+			multiplyNode.parent = -1;
+			multiplyNode.rightNode = tree.indexOf(currentNode);
+			// Fix children of /
+			let b = tree[currentNode.leftNode];
+			b.parent = tree.indexOf(currentNode);
+		}


When currentNodeIndex is 0, the code swaps multiplyNode and currentNode positions in the tree array. However, the multiplyNode.leftNode value was set to tree.indexOf(a) before the swap. After the swap at lines 496-497, this index may no longer point to the correct node if 'a' was also at index 0 or at the position where multiplyNode ends up. The leftNode reference should be updated after the swap to ensure it points to the correct node.

Copilot · 2026-01-05T18:22:31Z

pages/index/script.js

+		// Change current node index to index of * (to traverse over a)
+		currentNodeIndex = tree.indexOf(multiplyNode);
+


The variable currentNodeIndex is set to tree.indexOf(multiplyNode) at line 530, but then immediately overwritten at line 532 with findNextInDFS result. The assignment at line 530 appears to be dead code and serves no purpose. Either remove this line or clarify its intent in a comment.

Suggested change

// Change current node index to index of * (to traverse over a)

currentNodeIndex = tree.indexOf(multiplyNode);

// Move to the next node in DFS after the updated current node

Copilot · 2026-01-05T18:22:33Z

pages/index/script.js

+		a.parent = tree.indexOf(currentNode);
+
+		// Make / the right child of *
+		currentRightNode.rightNode = tree.indexOf(currentNode);
+		currentNode.parent = tree.indexOf(currentRightNode);


After tree manipulation that modifies indices by pushing new nodes and swapping positions, using tree.indexOf() to search for nodes can be inefficient and error-prone. The code relies on object reference equality, but if the tree is modified extensively, this could lead to unexpected behavior. Consider tracking indices directly or using a more robust identification system for nodes.

Copilot · 2026-01-05T18:22:33Z

pages/index/script.js

+// Essentially, check for divisions in form of ab/c (as these need to be normalised to a/b * c)
+// INPUTS: tree, node in tree
+// RETURNS: bool true if dividend is product, false if not
+function checkDividendIsProduct(tree, node)


The function name uses "dividend" but it should be "divisor". In the expression a/b, "a" is the dividend (numerator) and "b" is the divisor (denominator). This function checks if the right child (which represents "a" in the tree structure) is a product, but the comment and function name are misleading. The function should be named checkDivisorIsProduct or the comment should clarify the tree structure.

Copilot · 2026-01-05T18:22:33Z

pages/index/script.js

+// Checks whether current node is a /, and is dividend is something other than 1
+// Essentially, checks for divisions in the form of a/b (as these need to be normalised to 1/b * a)
+// INPUTS: tree, node in tree
+// RETURNS: bool true is dividend is not 1, false if it is
+function checkDividendIsNot1(tree, node)
+{
+	// Check if node is /, and right child is not 1


Similarly to checkDividendIsProduct, this function name uses "dividend" but should use "divisor" for mathematical accuracy. In a/b, the function checks if "a" (the right node) is not "1", but "a" would be the divisor in the tree representation, not the dividend. The function should be named checkDivisorIsNot1 for clarity.

Suggested change

// Checks whether current node is a /, and is dividend is something other than 1

// Essentially, checks for divisions in the form of a/b (as these need to be normalised to 1/b * a)

// INPUTS: tree, node in tree

// RETURNS: bool true is dividend is not 1, false if it is

function checkDividendIsNot1(tree, node)

{

// Check if node is /, and right child is not 1

// Checks whether current node is a /, and its divisor is something other than 1.

// Essentially, checks for divisions in the form of a/b (as these need to be normalised to 1/b * a).

// INPUTS: tree, node in tree

// RETURNS: bool true if divisor is not 1, false if it is

function checkDivisorIsNot1(tree, node)

{

// Check if node is /, and right child (the divisor) is not 1

Copilot · 2026-01-05T18:22:33Z

pages/index/script.js

+	let layers = {};
+	let maxLayer = 0;


Unused variable layers.

Suggested change

let layers = {};

let maxLayer = 0;

Copilot · 2026-01-05T18:22:34Z

pages/index/script.js

+
+	// Create a dictionary of depth:node indices
+	let layers = {};
+	let maxLayer = 0;


Unused variable maxLayer.

Suggested change

let maxLayer = 0;

Think it got there due to a poor rebase. Legacy code meant quotient normalisation wouldn't work at all.

In this transformation, we essentially swap the * and / nodes around. However, when / wasn't the root node, * wasn't getting correctly set as /'s parent's child. This has now been fixed.

Solution needs to be stored in normalised form. This was removed during testing, and has now been re-added

JoshdRod force-pushed the quotients-after-exponents branch from e62391d to bf8d4db Compare January 1, 2026 17:24

JoshdRod force-pushed the quotients-after-exponents branch from bf8d4db to f7fd0e6 Compare January 4, 2026 19:06

JoshdRod marked this pull request as ready for review January 4, 2026 19:06

JoshdRod force-pushed the main branch from ab88114 to 3904acd Compare January 4, 2026 20:37

JoshdRod requested a review from Jet-b January 4, 2026 21:34

JoshdRod added 6 commits January 4, 2026 21:47

<fix> Fix case where / is root node

ae6ada7

If / is root node of expression, the / and * need to swap. This is because the transformation ends up with the * being the logical root of the tree, and our tree traversal implementations rely on the root of the tree being at the front of the list.

<fix> Correct name of checkDividendIsProduct

b965a5a

Function checkDivisorIsProduct is renamed to checkDividendIsProduct, as that is what the function does. In the quotient a/b, a is the dividend, and b is the divisor.

<refactor> Remove redundant check in checkDividendIsProduct

c1b2bbb

A / node in a valid tree always has a right child, and we can assume that the input to this function is a valid tree. May need to add tree validation at some step though!

<fix> Fix case in a/b -> 1/b * a where / is root node

3612b86

See e5c7de0

<fix> Correctly add new * node to graph in a/b -> 1/b * a

434a74a

If / node wasn't the root node, then this pass would fail, because the created * node was not set as its parent's right node.

JoshdRod force-pushed the quotients-after-exponents branch from f7fd0e6 to 434a74a Compare January 4, 2026 21:47

JoshdRod requested a review from Copilot January 5, 2026 18:18

Copilot started reviewing on behalf of JoshdRod January 5, 2026 18:18 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

JoshdRod added 4 commits January 5, 2026 20:52

<fix> Remove legacy code

344bf13

Think it got there due to a poor rebase. Legacy code meant quotient normalisation wouldn't work at all.

<chore> Fix spelling mistake in comment

282ffeb

<fix> Make * node child of /s parent in ac/b -> a/b * c

8cedca7

In this transformation, we essentially swap the * and / nodes around. However, when / wasn't the root node, * wasn't getting correctly set as /'s parent's child. This has now been fixed.

<fix> Add normaliseTree back to solution

8439147

Solution needs to be stored in normalised form. This was removed during testing, and has now been re-added

	for (let i = 0; i < tree.length; i++)
	currentNodeIndex = rootNodeIndex;
	while (currentNodeIndex != -1)

	// RETURNS: bool true is dividend is not 1, false if it is
	// RETURNS: bool true if dividend is not 1, false if it is

		// Change current node index to index of * (to traverse over a)
		currentNodeIndex = tree.indexOf(multiplyNode);

	// Change current node index to index of * (to traverse over a)
	currentNodeIndex = tree.indexOf(multiplyNode);
	// Move to the next node in DFS after the updated current node

<fix> Normalise quotients to form a/b * c #23

Are you sure you want to change the base?

<fix> Normalise quotients to form a/b * c #23

Uh oh!

Conversation

JoshdRod commented Jan 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoshdRod commented Jan 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JoshdRod commented Jan 1, 2026 •

edited

Loading