Skip to content

Commit 6078bef

Browse files
kayjankayjan
committed
fix: modify logic is wrong when overriding is used with merge_children
1 parent f3214c3 commit 6078bef

File tree

3 files changed

+185
-6
lines changed

3 files changed

+185
-6
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99
- Tree Modify: Update documentation and docstring with some rephrasing.
1010
### Added:
1111
- Tree Modify: Add parameter `merge_attribute` to allow from-node and to-node attributes to be merged if there are clashes.
12+
### Fixed:
13+
- Tree Modify: Fixed bug when `merge_children` is used with `overriding` as the `merge_children` value is changed in for-loop (bad move, literally).
1214

1315
## [0.22.3] - 2024-11-14
1416
### Added:

‎bigtree/tree/modify.py‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,6 +1216,10 @@ def copy_or_shift_logic(
12161216

12171217
# Perform shifting/copying
12181218
for from_path, to_path in zip(from_paths, to_paths):
1219+
# Reset parameters
1220+
merge_children2 = merge_children
1221+
merge_leaves2 = merge_leaves
1222+
12191223
if with_full_path:
12201224
from_node = search.find_full_path(tree, from_path)
12211225
else:
@@ -1263,7 +1267,7 @@ def copy_or_shift_logic(
12631267
parent = to_node.parent
12641268
to_node.parent = None
12651269
to_node = parent
1266-
merge_children = False
1270+
merge_children2 = False
12671271
elif merge_attribute:
12681272
logging.info(
12691273
f"Path {to_path} already exists and their attributes will be merged"
@@ -1278,7 +1282,7 @@ def copy_or_shift_logic(
12781282
parent = to_node.parent
12791283
to_node.parent = None
12801284
to_node = parent
1281-
merge_children = False
1285+
merge_children2 = False
12821286
else:
12831287
logging.info(
12841288
f"Path {to_path} already exists and children are merged"
@@ -1303,7 +1307,7 @@ def copy_or_shift_logic(
13031307
parent = to_node.parent
13041308
to_node.parent = None
13051309
to_node = parent
1306-
merge_leaves = False
1310+
merge_leaves2 = False
13071311
else:
13081312
logging.info(
13091313
f"Path {to_path} already exists and leaves are merged"
@@ -1346,10 +1350,15 @@ def copy_or_shift_logic(
13461350
)
13471351

13481352
# Reassign from_node to new parent
1353+
# if merge_children and (overriding or merge_attribute):
1354+
# merge_children = False
1355+
# if merge_leaves and merge_attribute:
1356+
# merge_leaves = False
13491357
if copy:
13501358
logging.debug(f"Copying {from_node.node_name}")
13511359
from_node = from_node.copy()
1352-
if merge_children:
1360+
if merge_children2:
1361+
# overriding / merge_attribute handled merge_children, set merge_children=False
13531362
logging.debug(
13541363
f"Reassigning children from {from_node.node_name} to {to_node.node_name}"
13551364
)
@@ -1358,7 +1367,7 @@ def copy_or_shift_logic(
13581367
del children.children
13591368
children.parent = to_node
13601369
from_node.parent = None
1361-
elif merge_leaves:
1370+
elif merge_leaves2:
13621371
logging.debug(
13631372
f"Reassigning leaf nodes from {from_node.node_name} to {to_node.node_name}"
13641373
)

‎tests/tree/test_modify.py‎

Lines changed: 169 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,43 @@ def test_copy_nodes_merge_children_overriding(self):
610610
list(search.find_path(self.root, "/a/aa").children)
611611
), "Node parent deleted"
612612

613+
def test_copy_nodes_merge_children_overriding_multiple(self):
614+
new_aa = node.Node("aa", parent=self.root)
615+
new_bb = node.Node("bb", parent=new_aa)
616+
new_cc = node.Node("cc", age=1)
617+
new_cc.parent = new_bb
618+
new_dd = node.Node("dd", parent=new_aa)
619+
new_ee = node.Node("ee", age=1)
620+
new_ee.parent = new_dd
621+
bb2 = node.Node("bb", parent=self.root)
622+
cc2 = node.Node("cc2")
623+
cc2.parent = bb2
624+
625+
from_paths = ["/d", "/e", "/g", "/h", "/f"]
626+
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
627+
modify.shift_nodes(self.root, from_paths, to_paths)
628+
629+
from_paths = ["a/aa/bb", "a/aa/dd"]
630+
to_paths = ["/a/bb", "a/dd"]
631+
modify.copy_nodes(
632+
self.root, from_paths, to_paths, overriding=True, merge_children=True
633+
)
634+
assert search.find_path(
635+
self.root, "/a/bb/cc"
636+
), "Node children not merged, new children not present"
637+
assert not search.find_path(
638+
self.root, "/a/bb/cc2"
639+
), "Node children not merged, original children not overridden"
640+
assert (
641+
search.find_path(self.root, "/a/bb/cc").get_attr("age") == 1
642+
), f"Merged children not overridden, {export.print_tree(self.root)}"
643+
assert len(
644+
list(search.find_path(self.root, "/a/aa").children)
645+
), "Node parent deleted"
646+
assert search.find_path(
647+
self.root, "/a/ee"
648+
), "Node children not merged, new children not present"
649+
613650
# merge_children, merge_attribute
614651
def test_copy_nodes_merge_children_merge_attribute(self):
615652
from_paths = ["a/aa/bb"]
@@ -2169,6 +2206,67 @@ def test_copy_nodes_from_tree_to_tree_overriding_manual_check(self):
21692206
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong)
21702207
assert_tree_structure_node_root(self.root_other_full_wrong)
21712208

2209+
# merge_attribute
2210+
def test_copy_nodes_from_tree_to_tree_merge_attribute(self):
2211+
from_paths = ["d", "e", "g", "h", "f"]
2212+
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
2213+
modify.shift_nodes(self.root, from_paths, to_paths)
2214+
2215+
from_paths = ["a/b", "a/c"]
2216+
to_paths = ["a/b", "a/c"]
2217+
2218+
# Set attribute for node
2219+
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
2220+
self.root["b"].set_attrs({"gender": "b"})
2221+
2222+
modify.copy_nodes_from_tree_to_tree(
2223+
from_tree=self.root,
2224+
to_tree=self.root_other_full_wrong,
2225+
from_paths=from_paths,
2226+
to_paths=to_paths,
2227+
merge_attribute=True,
2228+
)
2229+
assert (
2230+
self.root_other_full_wrong["b"].get_attr("hello") == "world"
2231+
), "Original attribute not present"
2232+
assert (
2233+
self.root_other_full_wrong["b"].get_attr("gender") == "b"
2234+
), "Merge attribute not present"
2235+
2236+
def test_copy_nodes_from_tree_to_tree_merge_attribute_children(self):
2237+
from_paths = ["d", "e", "g", "h", "f"]
2238+
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
2239+
modify.shift_nodes(self.root, from_paths, to_paths)
2240+
2241+
from_paths = ["a/b", "a/c"]
2242+
to_paths = ["a/b", "a/c"]
2243+
2244+
# Set attribute for node
2245+
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
2246+
self.root_other_full_wrong["c"]["f"].set_attrs({"hello": "world"})
2247+
self.root["b"].set_attrs({"gender": "b"})
2248+
self.root["c"]["f"].set_attrs({"gender": "b"})
2249+
2250+
modify.copy_nodes_from_tree_to_tree(
2251+
from_tree=self.root,
2252+
to_tree=self.root_other_full_wrong,
2253+
from_paths=from_paths,
2254+
to_paths=to_paths,
2255+
merge_attribute=True,
2256+
)
2257+
assert (
2258+
self.root_other_full_wrong["b"].get_attr("hello") == "world"
2259+
), "Original attribute not present"
2260+
assert (
2261+
self.root_other_full_wrong["b"].get_attr("gender") == "b"
2262+
), "Merge attribute not present"
2263+
assert (
2264+
self.root_other_full_wrong["c"]["f"].get_attr("hello") == "world"
2265+
), "Original attribute of children not present"
2266+
assert (
2267+
self.root_other_full_wrong["c"]["f"].get_attr("gender") == "b"
2268+
), "Merge attribute of children not present"
2269+
21722270
# merge_children
21732271
def test_copy_nodes_from_tree_to_tree_merge_children(self):
21742272
from_paths = ["e", "g", "h"]
@@ -2330,10 +2428,40 @@ def test_copy_nodes_from_tree_to_tree_merge_children_overriding(self):
23302428
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong)
23312429
assert_tree_structure_node_root(self.root_other_full_wrong)
23322430

2431+
# merge_children, merge_attribute
2432+
def test_copy_nodes_from_tree_to_tree_merge_children_merge_attribute(self):
2433+
from_paths = ["d", "e", "g", "h", "f"]
2434+
to_paths = ["a/b/d", "a/b/e", "a/b/e/g", "a/b/e/h", "a/c/f"]
2435+
modify.shift_nodes(self.root, from_paths, to_paths)
2436+
2437+
from_paths = ["a/b", "a/c"]
2438+
to_paths = ["a/b", "a/c"]
2439+
2440+
# Set attribute for node
2441+
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
2442+
self.root["b"].set_attrs({"gender": "b"})
2443+
2444+
modify.copy_nodes_from_tree_to_tree(
2445+
from_tree=self.root,
2446+
to_tree=self.root_other_full_wrong,
2447+
from_paths=from_paths,
2448+
to_paths=to_paths,
2449+
merge_attribute=True,
2450+
merge_children=True,
2451+
)
2452+
assert_tree_structure_basenode_root(self.root_other_full_wrong)
2453+
assert_tree_structure_node_root(self.root_other_full_wrong)
2454+
assert (
2455+
self.root_other_full_wrong["b"].get_attr("hello") == "world"
2456+
), "Original attribute not present"
2457+
assert (
2458+
self.root_other_full_wrong["b"].get_attr("gender") == "b"
2459+
), "Merge attribute not present"
2460+
23332461
# merge_leaves, overriding
23342462
def test_copy_nodes_from_tree_to_tree_merge_leaves_overriding(self):
23352463
from_paths = ["d", "e", "g", "h", "f"]
2336-
to_paths = ["a/bb/d", "a/bb/e", "a/cc/g", "a/cc/h", "a/c/f"]
2464+
to_paths = ["a/bb/bb2/d", "a/bb/e", "a/cc/g", "a/cc/cc2/h", "a/c/f"]
23372465
modify.shift_nodes(self.root, from_paths, to_paths)
23382466

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

2481+
# merge_leaves, merge_attribute
2482+
def test_copy_nodes_from_tree_to_tree_merge_leaves_merge_attribute(self):
2483+
from_paths = ["d", "e", "g", "h", "f"]
2484+
to_paths = ["a/b/b2/d", "a/b/e", "a/cc/e/g", "a/cc/e/cc2/h", "a/c/f"]
2485+
modify.shift_nodes(self.root, from_paths, to_paths)
2486+
2487+
from_paths = ["a/b", "a/cc/e", "a/c"]
2488+
to_paths = ["a/b", "a/b/e", "a/c"]
2489+
2490+
# Set attribute for node
2491+
self.root_other_full_wrong["b"].set_attrs({"hello": "world"})
2492+
self.root_other_full_wrong["b"].extend([node.Node("e")])
2493+
self.root["b"].set_attrs({"gender": "b"})
2494+
self.root_other_full_wrong["c"]["f"].set_attrs({"hello": "world"})
2495+
self.root["c"]["f"].set_attrs({"gender": "b"})
2496+
2497+
modify.copy_nodes_from_tree_to_tree(
2498+
from_tree=self.root,
2499+
to_tree=self.root_other_full_wrong,
2500+
from_paths=from_paths,
2501+
to_paths=to_paths,
2502+
merge_leaves=True,
2503+
merge_attribute=True,
2504+
)
2505+
assert_tree_structure_basenode_root(self.root_other_full_wrong)
2506+
assert_tree_structure_basenode_root_attr(self.root_other_full_wrong)
2507+
assert_tree_structure_node_root(self.root_other_full_wrong)
2508+
assert (
2509+
self.root_other_full_wrong["b"].get_attr("hello") == "world"
2510+
), "Original attribute not present"
2511+
assert (
2512+
self.root_other_full_wrong["b"].get_attr("gender") == "b"
2513+
), "Merge attribute not present"
2514+
assert (
2515+
self.root_other_full_wrong["c"]["f"].get_attr("hello") == "world"
2516+
), "Original attribute of leaf not present"
2517+
assert (
2518+
self.root_other_full_wrong["c"]["f"].get_attr("gender") == "b"
2519+
), "Merge attribute of leaf not present"
2520+
23532521
# merge_children, merge_leaves
23542522
def test_copy_nodes_from_tree_to_tree_merge_children_and_leaf_error(self):
23552523
from_paths = ["a"]

0 commit comments

Comments
 (0)