Skip to content

Commit

Permalink
fix: modify logic is wrong when overriding is used with merge_children
Browse files Browse the repository at this point in the history
  • Loading branch information
@kayjan
kayjan committed Dec 25, 2024
1 parent f3214c3 commit 6078bef
Show file tree
Hide file tree
Showing 3 changed files with 185 additions and 6 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Tree Modify: Update documentation and docstring with some rephrasing.
### Added:
- Tree Modify: Add parameter `merge_attribute` to allow from-node and to-node attributes to be merged if there are clashes.
### Fixed:
- Tree Modify: Fixed bug when `merge_children` is used with `overriding` as the `merge_children` value is changed in for-loop (bad move, literally).

## [0.22.3] - 2024-11-14
### Added:
Expand Down
19 changes: 14 additions & 5 deletions bigtree/tree/modify.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,10 @@ def copy_or_shift_logic(

# Perform shifting/copying
for from_path, to_path in zip(from_paths, to_paths):
# Reset parameters
merge_children2 = merge_children
merge_leaves2 = merge_leaves

if with_full_path:
from_node = search.find_full_path(tree, from_path)
else:
Expand Down Expand Up @@ -1263,7 +1267,7 @@ def copy_or_shift_logic(
parent = to_node.parent
to_node.parent = None
to_node = parent
merge_children = False
merge_children2 = False
elif merge_attribute:
logging.info(
f"Path {to_path} already exists and their attributes will be merged"
Expand All @@ -1278,7 +1282,7 @@ def copy_or_shift_logic(
parent = to_node.parent
to_node.parent = None
to_node = parent
merge_children = False
merge_children2 = False
else:
logging.info(
f"Path {to_path} already exists and children are merged"
Expand All @@ -1303,7 +1307,7 @@ def copy_or_shift_logic(
parent = to_node.parent
to_node.parent = None
to_node = parent
merge_leaves = False
merge_leaves2 = False
else:
logging.info(
f"Path {to_path} already exists and leaves are merged"
Expand Down Expand Up @@ -1346,10 +1350,15 @@ def copy_or_shift_logic(
)

# Reassign from_node to new parent
# if merge_children and (overriding or merge_attribute):
# merge_children = False
# if merge_leaves and merge_attribute:
# merge_leaves = False
if copy:
logging.debug(f"Copying {from_node.node_name}")
from_node = from_node.copy()
if merge_children:
if merge_children2:
# overriding / merge_attribute handled merge_children, set merge_children=False
logging.debug(
f"Reassigning children from {from_node.node_name} to {to_node.node_name}"
)
Expand All @@ -1358,7 +1367,7 @@ def copy_or_shift_logic(
del children.children
children.parent = to_node
from_node.parent = None
elif merge_leaves:
elif merge_leaves2:
logging.debug(
f"Reassigning leaf nodes from {from_node.node_name} to {to_node.node_name}"
)
Expand Down
170 changes: 169 additions & 1 deletion tests/tree/test_modify.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,43 @@ def test_copy_nodes_merge_children_overriding(self):
list(search.find_path(self.root, "/a/aa").children)
), "Node parent deleted"

def test_copy_nodes_merge_children_overriding_multiple(self):
new_aa = node.Node("aa", parent=self.root)
new_bb = node.Node("bb", parent=new_aa)
new_cc = node.Node("cc", age=1)
new_cc.parent = new_bb
new_dd = node.Node("dd", parent=new_aa)
new_ee = node.Node("ee", age=1)
new_ee.parent = new_dd
bb2 = node.Node("bb", parent=self.root)
cc2 = node.Node("cc2")
cc2.parent = bb2

from_paths = ["/d", "/e", "/g", "/h", "/f"]
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
modify.shift_nodes(self.root, from_paths, to_paths)

from_paths = ["a/aa/bb", "a/aa/dd"]
to_paths = ["/a/bb", "a/dd"]
modify.copy_nodes(
self.root, from_paths, to_paths, overriding=True, merge_children=True
)
assert search.find_path(
self.root, "/a/bb/cc"
), "Node children not merged, new children not present"
assert not search.find_path(
self.root, "/a/bb/cc2"
), "Node children not merged, original children not overridden"
assert (
search.find_path(self.root, "/a/bb/cc").get_attr("age") == 1
), f"Merged children not overridden, {export.print_tree(self.root)}"
assert len(
list(search.find_path(self.root, "/a/aa").children)
), "Node parent deleted"
assert search.find_path(
self.root, "/a/ee"
), "Node children not merged, new children not present"

# merge_children, merge_attribute
def test_copy_nodes_merge_children_merge_attribute(self):
from_paths = ["a/aa/bb"]
Expand Down Expand Up @@ -2169,6 +2206,67 @@ def test_copy_nodes_from_tree_to_tree_overriding_manual_check(self):
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong)
assert_tree_structure_node_root(self.root_other_full_wrong)

# merge_attribute
def test_copy_nodes_from_tree_to_tree_merge_attribute(self):
from_paths = ["d", "e", "g", "h", "f"]
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
modify.shift_nodes(self.root, from_paths, to_paths)

from_paths = ["a/b", "a/c"]
to_paths = ["a/b", "a/c"]

# Set attribute for node
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
self.root["b"].set_attrs({"gender": "b"})

modify.copy_nodes_from_tree_to_tree(
from_tree=self.root,
to_tree=self.root_other_full_wrong,
from_paths=from_paths,
to_paths=to_paths,
merge_attribute=True,
)
assert (
self.root_other_full_wrong["b"].get_attr("hello") == "world"
), "Original attribute not present"
assert (
self.root_other_full_wrong["b"].get_attr("gender") == "b"
), "Merge attribute not present"

def test_copy_nodes_from_tree_to_tree_merge_attribute_children(self):
from_paths = ["d", "e", "g", "h", "f"]
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
modify.shift_nodes(self.root, from_paths, to_paths)

from_paths = ["a/b", "a/c"]
to_paths = ["a/b", "a/c"]

# Set attribute for node
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
self.root_other_full_wrong["c"]["f"].set_attrs({"hello": "world"})
self.root["b"].set_attrs({"gender": "b"})
self.root["c"]["f"].set_attrs({"gender": "b"})

modify.copy_nodes_from_tree_to_tree(
from_tree=self.root,
to_tree=self.root_other_full_wrong,
from_paths=from_paths,
to_paths=to_paths,
merge_attribute=True,
)
assert (
self.root_other_full_wrong["b"].get_attr("hello") == "world"
), "Original attribute not present"
assert (
self.root_other_full_wrong["b"].get_attr("gender") == "b"
), "Merge attribute not present"
assert (
self.root_other_full_wrong["c"]["f"].get_attr("hello") == "world"
), "Original attribute of children not present"
assert (
self.root_other_full_wrong["c"]["f"].get_attr("gender") == "b"
), "Merge attribute of children not present"

# merge_children
def test_copy_nodes_from_tree_to_tree_merge_children(self):
from_paths = ["e", "g", "h"]
Expand Down Expand Up @@ -2330,10 +2428,40 @@ def test_copy_nodes_from_tree_to_tree_merge_children_overriding(self):
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong)
assert_tree_structure_node_root(self.root_other_full_wrong)

# merge_children, merge_attribute
def test_copy_nodes_from_tree_to_tree_merge_children_merge_attribute(self):
from_paths = ["d", "e", "g", "h", "f"]
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
modify.shift_nodes(self.root, from_paths, to_paths)

from_paths = ["a/b", "a/c"]
to_paths = ["a/b", "a/c"]

# Set attribute for node
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
self.root["b"].set_attrs({"gender": "b"})

modify.copy_nodes_from_tree_to_tree(
from_tree=self.root,
to_tree=self.root_other_full_wrong,
from_paths=from_paths,
to_paths=to_paths,
merge_attribute=True,
merge_children=True,
)
assert_tree_structure_basenode_root(self.root_other_full_wrong)
assert_tree_structure_node_root(self.root_other_full_wrong)
assert (
self.root_other_full_wrong["b"].get_attr("hello") == "world"
), "Original attribute not present"
assert (
self.root_other_full_wrong["b"].get_attr("gender") == "b"
), "Merge attribute not present"

# merge_leaves, overriding
def test_copy_nodes_from_tree_to_tree_merge_leaves_overriding(self):
from_paths = ["d", "e", "g", "h", "f"]
to_paths = ["a/bb/d", "a/bb/e", "a/cc/g", "a/cc/h", "a/c/f"]
to_paths = ["a/bb/bb2/d", "a/bb/e", "a/cc/g", "a/cc/cc2/h", "a/c/f"]
modify.shift_nodes(self.root, from_paths, to_paths)

from_paths = ["a/bb", "a/cc", "a/c"]
Expand All @@ -2350,6 +2478,46 @@ def test_copy_nodes_from_tree_to_tree_merge_leaves_overriding(self):
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong, c=("c", 1))
assert_tree_structure_node_root(self.root_other_full_wrong)

# merge_leaves, merge_attribute
def test_copy_nodes_from_tree_to_tree_merge_leaves_merge_attribute(self):
from_paths = ["d", "e", "g", "h", "f"]
to_paths = ["a/b/b2/d", "a/b/e", "a/cc/e/g", "a/cc/e/cc2/h", "a/c/f"]
modify.shift_nodes(self.root, from_paths, to_paths)

from_paths = ["a/b", "a/cc/e", "a/c"]
to_paths = ["a/b", "a/b/e", "a/c"]

# Set attribute for node
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
self.root_other_full_wrong["b"].extend([node.Node("e")])
self.root["b"].set_attrs({"gender": "b"})
self.root_other_full_wrong["c"]["f"].set_attrs({"hello": "world"})
self.root["c"]["f"].set_attrs({"gender": "b"})

modify.copy_nodes_from_tree_to_tree(
from_tree=self.root,
to_tree=self.root_other_full_wrong,
from_paths=from_paths,
to_paths=to_paths,
merge_leaves=True,
merge_attribute=True,
)
assert_tree_structure_basenode_root(self.root_other_full_wrong)
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong)
assert_tree_structure_node_root(self.root_other_full_wrong)
assert (
self.root_other_full_wrong["b"].get_attr("hello") == "world"
), "Original attribute not present"
assert (
self.root_other_full_wrong["b"].get_attr("gender") == "b"
), "Merge attribute not present"
assert (
self.root_other_full_wrong["c"]["f"].get_attr("hello") == "world"
), "Original attribute of leaf not present"
assert (
self.root_other_full_wrong["c"]["f"].get_attr("gender") == "b"
), "Merge attribute of leaf not present"

# merge_children, merge_leaves
def test_copy_nodes_from_tree_to_tree_merge_children_and_leaf_error(self):
from_paths = ["a"]
Expand Down

0 comments on commit 6078bef

Please sign in to comment.