Throw exception on attempt to access attr of nonexistent edge object

[jafingerhut]

This is one possible change that could be made to address issue #48.
It also adds many tests for attribute functions, both for existing and
non-existent nodes and edges, in directed and undirected graphs. For
undirected graphs, it tries variations of accessing both edge
directions, to ensure that they both behave as "the same edge".

Also fix one arity bug.

[jafingerhut]

The proposed change to function resolve-node-or-edge is likely the most controversial part of this PR, but I am happy to consider all feedback you may have. That change to resolve-node-or-edge would resolve issue #48, causing an exception to be thrown if you call any attribute-accessing function with an edge object that does not exist in the graph.

[Engelberg]

Thanks for this. Although resolve-node-or-edge is still "constant time", as I began to merge it, I found myself uncomfortable with adding the extra overhead of verifying the edge's existence every time you want to update attributes on an edge, since 1) I guess it's a low probability to use an edge that comes from another graph and 2) It doesn't actually "break" things if you do it, it's just sort of a no-op that produces extra clutter in the graph, which is not what the user intended.

I'd still be open to merging this if you or anyone else reported that you actually got burned by the current behavior, thinking you were manipulating attributes on an edge when it was doing nothing because the edge accidentally came from another graph. I just don't want to do it if it is only hypothetical. Although ubergraph is a purely functional data structure, I think most people will compare its performance to mutable graphs and want it to be as performant as is feasible. So I rate performance here as somewhat more important than protecting against a low-probability garbage-in-garbage-out scenario. From a performance standpoint, another thing I'd like to explore at some point is using specter to speed up the heavy usage of get-in, assoc-in and update-in. I think if I ever get those improvements in place, I'll be more comfortable with merging this pull request because the deep get-in call to get the edge set would be even more negligible. Also, specter would make it way easier to do things like removing empty attribute maps when you've removed all the attributes, avoiding clutter. Right now, those sorts of space optimizations aren't being done because it's so awkward to write the code to do that without something like specter.

Also, if there are tests in this pull request that you think should be incorporated irrespective of the main proposed change, I would certainly merge those. (I noticed while reviewing this pr that you caught a missing g from a call to get-edge in remove-attrs, which I've manually fixed).

[yuhan0]

I'd still be open to merging this if you or anyone else reported that you actually got burned by the current behavior, thinking you were manipulating attributes on an edge when it was doing nothing because the edge accidentally came from another graph.

I'm not sure if this counts as the same issue, but I did run into this case quite a number of times using the arity (uber/attr g n1 n2 k) when there was no edge between the two nodes, and an exception was thrown without a helpful message:

(let [g (-> (graph [1 2] [2 3])
            (add-attr 1 2 :foo :bar))]
  (attr g 1 3 :foo))

;; resulting error:
Execution error (IllegalArgumentException) at ubergraph.core/resolve-node-or-edge (core.clj:519).
Invalid node or edge description:

Changing it to throw an exception right away made for a much easier debugging experience:

ubergraph/src/ubergraph/core.clj

Line 174 in 45553ac

(add-attr [g n1 n2 k v] (add-attr g (get-edge g n1 n2) k v))

=>

  (attr [g n1 n2 k]
        (if-let [e (get-edge g n1 n2)]
          (attr g e k)
          (throw (IllegalArgumentException.
                  (str "Invalid node or edge description: " n1 " " n2)))))