Mesh.vertex_attribute doesn't work ass expected

[Achillx]

Describe the bug I might be tired, or stupid... While looping through all vertices of a mesh and updating a vertex attribute of one vertex, all the vertices in the loop also get that value.

To Reproduce Run the following code:

from compas.geometry import Box
from compas.datastructures import Mesh

# make a simple compas mesh
mesh = Mesh.from_shape(Box.from_width_height_depth(2, 2, 2))

# add a new vertex attribute to all vertices
mesh.update_default_vertex_attributes({"new_attr": []})

# add a value to this new attribute only to some vertices
for index, v_attributes in enumerate(mesh.vertices(data=True)):
    print("------", index)
    v_key, v_data = v_attributes
    attribute = mesh.vertex_attribute(v_key, 'new_attr')
    print('before', attribute)
    if index % 2 == 0:
        attribute.append('why_{}'.format(index))
    print('after', mesh.vertex_attribute(v_key, 'new_attr'))

The output I get is:

------ 0
before []
after ['why_0']
------ 1
before ['why_0']
after ['why_0']
------ 2
before ['why_0']
after ['why_0', 'why_2']
------ 3
before ['why_0', 'why_2']
after ['why_0', 'why_2']
------ 4
before ['why_0', 'why_2']
after ['why_0', 'why_2', 'why_4']
------ 5
before ['why_0', 'why_2', 'why_4']
after ['why_0', 'why_2', 'why_4']
------ 6
before ['why_0', 'why_2', 'why_4']
after ['why_0', 'why_2', 'why_4', 'why_6']
------ 7
before ['why_0', 'why_2', 'why_4', 'why_6']
after ['why_0', 'why_2', 'why_4', 'why_6']

Expected behavior I'm expecting to change the attribute only of the desired vertex/vertices. An output like the following:

------ 0
before []
after ['why_0']
------ 1
before []
after []
------ 2
before []
after ['why_2']
------ 3
before []
after []
------ 4
before []
after ['why_4']
------ 5
before []
after []
------ 6
before []
after ['why_6']
------ 7
before []
after []

Desktop (please complete the following information):

• OS: macOS Catalina
• Python version: 3.7.6
• Python package manager: Conda
• Compas: 0.19.1

[gonzalocasas]

This is actually expected behavior from python. The empty list you pass as default is a reference. It's the same assigned to all attributes effectively (the attributes are getting the memory reference to that list, not copies of the list).

Despite this being somewhat confusing, I would be tempted to leave as is because it's also what happens when you use an empty list as the default value of a function argument.

[tomvanmele]

@gonzalocasas smack on the money, in my opinion!

[beverlylytle]

Here's a variation on your code that behaves the way you want it to.

from compas.geometry import Box
from compas.datastructures import Mesh

# make a simple compas mesh
mesh = Mesh.from_shape(Box.from_width_height_depth(2, 2, 2))

# add a new vertex attribute to all vertices
mesh.update_default_vertex_attributes({"new_attr": []})

# add a value to this new attribute only to some vertices
for index, v_attributes in enumerate(mesh.vertices(data=True)):
    print("------", index)
    v_key, v_data = v_attributes
    attribute = mesh.vertex_attribute(v_key, 'new_attr')
    print('before', attribute)
    if index % 2 == 0:
        mesh.vertex_attribute(v_key, 'new_attr', attribute + ['why_{}'.format(index)])
    print('after', mesh.vertex_attribute(v_key, 'new_attr'))

Of course, this only performs a shallow copy of attribute. If you initialize with {"new_attr": [[]]} and then later call attribute[0].append('why?!?!') you'll run into the same problem.

[tomvanmele]

the behaviour is indeed typical Python. and one could argue that providing an empty list as default attribute value is a bit nonsensical: as recommended in Python, if you want to provide an empty list as default parameter to a function (without the intention of modifying the original list) you should provide None and use something like param = param or [] at the beginning of the function body.

nevertheless, the following is indeed tempting:

mesh = Mesh.from_obj(compas.get('faces.obj'))
mesh.update_default_vertex_attributes({'loads': [0, 0, 0]})

vertex = mesh.get_any_vertex()
loads = mesh.vertex_attribute(vertex, 'loads')
loads[2] = 2

for vertex in mesh.vertices():
    loads = mesh.vertex_attribute(vertex, 'loads')
    print(loads)

this will result in all loads having the item at index 2 to be 2.

one could argue that the correct way of setting a single attribute is to actually assign a value and not just update (part of) the default. therefore, the correct way of doing this could be

vertex = mesh.get_any_vertex()
mesh.vertex_attribute(vertex, 'load', [0, 0, 2])

however, this eliminates the possibility of concisely incrementing, for example, only the z value without affecting the other values

vertex = mesh.get_any_vertex()
load = mesh.vertex_attribute(vertex, 'load')
load[2] += 1

to avoid this something like the following is of course possible, but perhaps not ideal

vertex = mesh.get_any_vertex()
load = mesh.vertex_attribute(vertex, 'load')
mesh.vertex_attribute(vertex, 'load', [load[0], load[1], load[2] + 1])

the best way around this is to never assign mutable objects as default attributes, unless the above behaviour is what you're after (like in Python).

mesh.update_default_vertex_attributes({'px': 0, 'py': 0, 'pz': 0})

[beverlylytle]

This last suggestion still has shallow copy issues (if the elements of load are again lists). To get around this, one would have to use copy.deepcopy, or as you suggest, use something immutable, which, if an iterable is needed, would be a tuple or a frozenset.

vertex = mesh.get_any_vertex()
load = mesh.vertex_attribute(vertex, 'load')
new_load = copy.deepcopy(load)
new_load[2] += 1
mesh.vertex_attribute(vertex, 'load', new_load)

[Achillx]

My workaround was kind of what @beverlylytle suggested. I make a copy of the previous version, change whatever needs to be changed in the copy, and update the attribute using the copied version.

Probably my case is a bit of a niche use but... anyway, the reason behind it is the following: I have a flat mesh and some circles that sometimes overlap. I want each vertex to know inside which circle/circles it lies. That is why the empty list (== inside no circle). I could turn the default attribute value into None and if the vertex is inside a circle it is changed into a list. This way the next time I need to append another circle, the list is unique to that vertex.

Am I correct?

[gonzalocasas]

Yep, that's correct 👌

[tomvanmele]

not sure if we need to modify the behaviour (dealing with mutability on a per-attribute basis sounds like a bad idea) but i will add the side-effects of the current implementation to the docs...

[gonzalocasas]

random idea: would it make sense to allow setting a function as default value, which acts as the constructor for complex types? Eg.

# as an anonymous function:
mesh.update_default_vertex_attributes({"new_attr": lambda: []})

# or as a named function
def create_default_box():
    return Box.from_width_height_depth(1, 1, 1)

mesh.update_default_vertex_attributes({"another_new_attr": create_default_box})

this would make the behavior requested in this issue possible without convoluting the client code (at the expense of a bit of extra complexity in the internals of attribute views, having to type-check and execute functions when a default is requested).

[tomvanmele]

anyone willing to convert this into a PR?