Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 46 by adding metadata g to all returned edge objects #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jafingerhut
Copy link
Contributor

If we modify find-edges-impl, it will also cause all of the functions
listed below to return edges with metadata g, since they all end up
calling find-edges-impl:

  • find-edge-impl
  • find-edges
  • find-edge
  • get-edge

If we modify find-edges-impl, it will also cause all of the functions
listed below to return edges with metadata g, since they all end up
calling find-edges-impl:

* find-edge-impl
* find-edges
* find-edge
* get-edge
@Engelberg
Copy link
Owner

Thanks. Since this would change the function's return type from a set to a seq, I need to do some double-checking to be sure that's not a problem.

@Engelberg
Copy link
Owner

For background, I think the reason I didn't bother with attaching the metadata here is that originally, that was sort of a hack so that I when passed graphs to loom algorithm's, which frequently destructure an edge as [src dest weight], there would be a way to pull the weight out of an edge for the destructuring. Since it was only really meant to work in the context of loom algorithms, the find-edges protocol, which is unique to ubergraph, didn't seem like something to address.

But, now there are a couple other places (like build-graph) where adding an Edge object to a graph that came from another graph feels most natural if you bring the attributes along with it. I've never personally used that feature (adding Edge object from one graph to another), but it seems like something potentially worth supporting. So that's why I would agree that this is a problem I'd like to address. Otherwise, I should probably scrap that feature.

@Engelberg
Copy link
Owner

Looking at it, I guess it doesn't currently promise a set return value in all possible code branches.

@jafingerhut
Copy link
Contributor Author

I don't actually have a use case in mind for taking edge objects from one graph and adding them directly to another. There seem to be at least a few subtleties with using edges obtained from one graph in calls to others (as this issue demonstrates #48), but you are right that for build-graph perhaps it makes most sense, assuming that the new graph get new UUIDs for edges added to it.

That said, it is certainly no problem for me to close this if we find that the changes are not useful.

@Engelberg
Copy link
Owner

After doing some soul-searching on this issue over the past couple of weeks, I've come to the conclusion that it's a bad idea for end users to expect the edges to have graph and therefore attribute info in the edge's metadata. This creates the wrong mental model, where users will imagine the attributes belong to the edge, rather than correctly programming to the abstraction that the attributes are stored in the graph itself.

It's still going to be necessary to leave that metadata hack in place on the protocols that loom uses, so that those algorithms can get at the weight via destructuring. But I don't want this to be a feature of the ubergraph API.

My current plan is:

  1. Revert the changes I made in the last update where I expanded the set of functions where you could pass Edge objects instead of edge descriptions.
  2. Throw errors in the places where Edge objects are not accepted.
  3. Think about whether it makes sense to remove Edge objects as "inits" for build-graph (ideally would like to take it out now, but it may still be necessary as an implementation detail for being able to use a whole other graph as an init, which is something I do want to support; also, want to be careful about breaking existing code).
  4. Add some convenience functions like maybe node-with-attrs and edge-with-attrs that return [node attribute-map] and [src dest attribute-map] vectors that can be easily destructured and can be used conveniently as inits in build-graph.

Will be working on this over the next couple of days and will then cut a release with these changes and the other pull request I just merged, so further comments welcome.

@Engelberg
Copy link
Owner

Engelberg commented Aug 13, 2019

I have completed the changes to code and README to prevent edge objects from being used in graph constructors, and added node-with-attrs and edge-with-attrs. Take a look and let me know what you think, and then I'll push this as version 0.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants