Question: No 'name' attribute for network items

[krooq]

BindsNET has a really nice API, its super easy to understand and has a low barrier for entry, massive props to the team.
I found one rough edge that I'd like to understand a little better and maybe propose a change,
i.e. network items have no 'name' attribute.

This example code is a really good starting point and this is what I have been using to learn bindsnet.
I copied the code into my editor and messed around a bit and found after a while that it was getting a bit ugly slinging all these strings around.

I ended up doing something like this to make the code much cleaner and reduce the string duplication

class Net(Network):
    def add_layer(self, nodes: Nodes, name: str) -> str:
        super().add_layer(nodes, name)
        setattr(nodes, 'name', name)
        return nodes

    def add_connection(self, connection: Connection) -> Connection:
        super().add_connection(connection, source=connection.source.name, target=connection.target.name)
        return connection

    def add_monitor(self, monitor: Monitor, name: str) -> Monitor:
        super().add_monitor(monitor, name)
        return monitor

This made the network creation just a few lines and meant I didn't have to sling around all those strings.

time = 500
net = Net()
layer_0 = net.add_layer(Input(n=100), 'layer_0')
layer_1 = net.add_layer(LIFNodes(n=1000), 'layer_1')
cxn_0_1 = net.add_connection(Connection(layer_0, layer_1, w=0.05 + 0.1 * torch.randn(layer_0.n, layer_1.n)))
cxn_1_1 = net.add_connection(Connection(layer_1, layer_1, w=0.025 * (torch.eye(layer_1.n) - 1)))
mon_0   = net.add_monitor(Monitor(layer_0, ("s",), time), 'mon_0')
mon_1   = net.add_monitor(Monitor(layer_1, ("s","v"), time), 'mon_1')

Admittedly there are less intense ways to do this like just creating a separate string for layer names but it didn't feel that appealing.

So a few questions
Is it worth adding a name attribute to items that can be attached to a network to clean up the API?
Is my example just a contrived case that doesn't reflect the real world?

[djsaunde]

Hi @krooq,

Thanks for the using BindsNET, and thanks for the feedback!

To answer your questions:

Is it worth adding a name attribute to items that can be attached to a network to clean up the API?

Yes. In fact, I really like this change. I agree that there is redundancy in the naming logic at the moment.

Is my example just a contrived case that doesn't reflect the real world?

IIUC, this change should cover all use cases. Once a layer is named, the layer's name attribute can be used to automatically name connections.

If everyone else agrees that this is a good change, would you mind putting together a PR with this change? On top of updating the bindsnet.network logic, we'll have to update the examples, tests, and documentation to reflect this change.

[krooq]

Sure I can give it a go.
Pretty new to BindsNET so it might take me a little while but it would be a good way to understand it all a bit better.

[Hananel-Hazan]

Thank you for this welcome suggestion. You are right, there is some redundancy in the way we defining the network. Some of this style of code comes from other projects that we wanted to be compatible with their code style.

As Dan mentioned, this change should cause major changes in many parts of the project. The transition will take time.

[krooq]

I was wondering if it would be better to give users the choice of accessing network items using either the name or object itself.
e.g. def add_connection(self, connection: AbstractConnection, source: Union[str, Nodes], target: Union[str, Nodes])
I understand this isn't very Pythonic (i.e. more than 1 way to do it) but it comes with a few advantages.

1. It can be clearly expressed in the types so it wouldn't really be confusing
2. It would allow for migration away from strings at a later time (if desired)
3. It wouldn't be a breaking change on the public API afaik

But I do not want to complicate things either.
This would require a lot more effort and add an extra maintenance burden.

[djsaunde]

@krooq I think I like that idea as well. Could you explain why we might want to move away from strings?

Random idea: automatic naming of layers / monitors, for when the user fails to pass in names (e.g., LIFNodes_1, LIFNodes_2, ..., and so on for other layer types) (e.g., LIFNodes_1_Monitor, etc.).

[krooq]

@djsaunde in Python (most other languages) objects have a unique int identifier id(), this is managed by the runtime, I don't think its mutable. It's just more robust and concise to use this built in identifier for equality than a string.
It also separates the naming semantics from the identification if for some reason the user wanted to rename nodes (maybe a GUI editor would have this feature).

I'm not an expert on the use cases of all the features of BindsNET, perhaps having a string key for Nodes advantageous especially for those that are less exposed to code.
Since most of the time users will only be working in one file it's probably unnecessary to change the way the Nodes are keyed (i.e. replace the strings).

I think the automatic naming could be a neat feature.
It would mean the naming is tied to the context of the network (as the index of the layer is required to produce the name). Although I don't think this is a bad thing, that's how it works at the moment.
It just means that the name can't live on the Nodes object, it will have to be owned by the network.
It could look something like this:

    def add_layer(self, layer: Nodes, name: str = None) -> None:
       # ask the layer for a new name based on the nb of existing layers
        label = name if name is not None else layer.new_name(len(self.layers))
        self.layers[label] = layer
...

    def add_connection(
        self, connection: AbstractConnection, source: Union[str,Nodes], target: Union[str,Nodes]
    ) -> None:
        # either use the given strings as connection keys or get the layer keys from the dict (assumes layers are unique objects)
        source_str = source if type(source) == str else self.layers.keys()[self.layers.values().index(source)]
        target_str = target if type(target) == str else self.layers.keys()[self.layers.values().index(target)]
        self.connections[(source_str, target_str)] = connection
        self.add_module(source_str + "_to_" + target_str, connection)
...

[djsaunde]

@djsaunde in Python (most other languages) objects have a unique int identifier id(), this is managed by the runtime, I don't think its mutable. It's just more robust and concise to use this built in identifier for equality than a string.
It also separates the naming semantics from the identification if for some reason the user wanted to rename nodes (maybe a GUI editor would have this feature).

I'm not an expert on the use cases of all the features of BindsNET, perhaps having a string key for Nodes advantageous especially for those that are less exposed to code.
Since most of the time users will only be working in one file it's probably unnecessary to change the way the Nodes are keyed (i.e. replace the strings).

Okay, thanks for the explanation. I think we'll table this idea for now, but I like the discussion.

I think the automatic naming could be a neat feature.
It would mean the naming is tied to the context of the network (as the index of the layer is required to produce the name). Although I don't think this is a bad thing, that's how it works at the moment.
It just means that the name can't live on the Nodes object, it will have to be owned by the network.

Yup, that all makes sense. I'll create a separate GitHub issue for this (labeled as an "enhancement") for any discussion related to it.