I think we should look more closely at established convention for the top/bottom arguments. It looks like top_nodes= is frequently used, and bottom_nodes is implied (so it's not a keyword argument).

Also, when adding new keyword arguments, we sometimes consider making them keyword only by using e.g. *, top_nodes=None in the signature. @rossbar, do you think keyword-only makes sense here?

Good suggestions -- at least from my perspective:

• only input top_nodes
• make that new input keyword-only

I wonder if we have in the contributing docs that we prefer keyword only for new parameters. We should have that somewhere. :)

I think we should look more closely at established convention for the top/bottom arguments. It looks like top_nodes= is frequently used, and bottom_nodes is implied (so it's not a keyword argument).

I think this is generally true, though IIUC the goal here is to provide a label for the nodes (i.e. instead of integers) in which case I wonder if the better approach isn't to break from the nodelist pattern and instead use an explicit mapping (though this assumes that the user will know what the integer indices will be a priori)

Regarding top_nodes vs. top_nodelist - top_nodes is reasonable choice, but I do think it's important to consider the conventions found in convert_matrix.py associated with creating graphs from adjacency matrices as well. There, they use nodelist for the analogous argument. The list part of the name also helps convey some information to the user - namely, that in this function the order of the input nodes matters - since top_nodelist(bottom_nodelist) will be mapped to the indices of A's rows(columns). That's not the case elsewhere within the module, so helping to highlight it here might be beneficial.

Additionally, I do think we need to keep the bottoms_nodes argument. Without it, users will only be able to provide custom labels to the top nodes of the output graph, corresponding to the rows of the input biadjacency matrix (since rows and columns here correspond to two different sets of nodes).

Ah, crap. Did not refresh my page recently enough to see @rossbar's comment.

though IIUC the goal here is to provide a label for the nodes (i.e. instead of integers)

Yep, this is the goal here.

in which case I wonder if the better approach isn't to break from the nodelist pattern and instead use an explicit mapping (though this assumes that the user will know what the integer indices will be a priori)

This sounds like it could be a good idea to me. In any implementation, the user will need to know the indices a priori anyhow to meaningfully construct the nodelists with correct ordering. I suppose mapping could add additional confusion due to taking the integer of the node for the first row/column to 1 rather than 0, leading to an off-by-one error.

All good points @LorentzFactor. Thank you!
We are indicating more than which node is in which set -- we are indicating the order of the nodes in each set.
So indeed -- we need two input parameters and the phrase nodelist in the names makes good sense too.