Misleading method name from_list

[bauerfe]

The name of the method NIRGraph.from_list suggests that a list is expected as input:

graph = NIRGraph.from_list([node_a, node_b, node_c])

In reality however it expects the nodes to be passed as individual arguments:

graph = NIRGraph.from_list(node_a, node_b, node_c)

I suggest the method to be either renamed to something like "Compose" or "Sequential", or to change it such that it accepts a list.

[matjobst]

I agree that this is slightly misleading.
Our current implementation is similar to the torch.nn.Sequential of Pytorch. Tensorflow's tf.keras.Sequential is similar to your first suggestion with the lists.
I like the idea of renaming to Sequential and keeping the definition otherwise as is.

[Jegp]

Nicely spotted! That's my bad. I really like avoiding the two [ ] characters :P

How about adding a check so we can work with both lists and varargs? That's also what PyTorch is doing, I believe.

[bauerfe]

I would also prefer renaming it to Sequential and passing the args without container.

Pytorch (at least my possibly outdated version) raises an error when passing a list:

TypeError: list is not a Module subclass

I also don't mind supporting both, though.

[Jegp]

This should be fixed with #50. Would you mind confirming and closing, @bauerfe ?

[bauerfe]

I'm really sorry but I must be missing something. Could you please specify how #50 addresses this issue? Neither the name nor the call signature of the from_list method has changed.

[Jegp]

You're entirely correct that the PR doesn't change the name of the method. But it allows the from_list method to take a list and varargs (

NIR/nir/ir.py

Line 44 in 464de9f

if len(nodes) > 0 and (

). So it kinda addresses the issue that the method called from_list doesn't even take lists.

I'm sympathetic if you wish to rename the method if you think there's a better name. I believe you suggested sequence or compose?

[Jegp]

@bauerfe could you share your opinion on how to proceed? Is the current naming sufficient, given #50?

[bauerfe]

Yeah, sorry for the late reply. I think this is fine.