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

Use Attributes for all mathematical properties (weight, balance etc.) #114

Closed
clue opened this issue Dec 31, 2014 · 4 comments · Fixed by #182
Closed

Use Attributes for all mathematical properties (weight, balance etc.) #114

clue opened this issue Dec 31, 2014 · 4 comments · Fixed by #182

Comments

@clue
Copy link
Member

clue commented Dec 31, 2014

See #103.

@clue
Copy link
Member Author

clue commented Oct 12, 2015

This ticket will remove all mathematical properties from the Vertex and Edge classes, such as weight, balance etc. and will use general purpose Attributes instead.

This means that all of these will have to be set via attributes in the future:

// old
$edge->setWeight(10.5);

// new
$edge->setAttribute('weight', 10.5);

This has several consequences:

  • A big BC break obviously :-)
  • Attribute type validation is no longer possible ("weight" could be a string)
  • Attribute constraints are no longer possible ("flow" could exceed "capacity")
  • Attribute names are not standardized in any way
    • We should probably reserve a "well known namespace" or similar
    • Algorithms classes need a way to set the required attribute names

I'll look into this to see how it works out.

@cyberscribe
Copy link

It seems to me that retaining getWeight/setWeight as an interface to the arbitrary attributes in a future version would allow you to retain BC.

While GraphML stores weights as an arbitrary attribute in representation, in practice the weight value is highly significant in that it differentiates a graph from a network. Tools like Gephi respect it as significant and change how they render the graph based on its presence.

So, I think there is a big difference between whether a variable is "special" in terms of how it gets stored or rendered, and whether it is "special" in terms of how it gets used. To my mind, weight is "special" in practice, and so retaining getters/setters for it (that can transparently manipulate the arbitrary storage/rendering) would be a "best of both worlds" scenario.

Speaking as an end-user for a moment, the package seems fairly feature-complete to me, so if a future release a) breaks BC and b) doesn't give me compelling new features, I would be disinclined to follow that path.

@clue
Copy link
Member Author

clue commented Jul 15, 2016

It seems to me that retaining getWeight/setWeight as an interface to the arbitrary attributes in a future version would allow you to retain BC.

Thanks for your input, I think this is a very valid point and we should consider keeping BC for the next version at least 👍

We will very likely still deprecate these methods and remove them in future versions, but keeping them for an intermediary release certainly makes sense 👍

While GraphML stores weights as an arbitrary attribute in representation, in practice the weight value is highly significant in that it differentiates a graph from a network. Tools like Gephi respect it as significant and change how they render the graph based on its presence.

I agree that edge weight (and family) is a significant attribute in certain cases, as that was also the main motivation to add them in the first place. However, we've seen several people ask for edges with multiple weights and other special attributes not possibly with the fixed properties. Besides, depending on your context, certain other attributes will be "significant", such as GraphViz layout attributes, which have already been removed in the previous release.

I understand that this may be frustrating as it require additional work, but this way we provide a clear upgrade path and plenty of time (ETA at least 6 months) to prepare for this change.

@clue clue modified the milestones: v0.10.0, v0.11.0 Sep 29, 2018
@clue
Copy link
Member Author

clue commented Sep 29, 2018

This ticket hasn't seen any updates in quite a while and I suppose it's about time for a progress update at least: I'm still planning to remove these attributes, but will postpone this to a later release simply because a release has been outstanding for quite some time and I would rather not introduce too many BC breaks in a single release. This should also give people plenty of time to plan ahead and be aware that they might be relying on a deprecated feature that will be removed with the following release.

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

Successfully merging a pull request may close this issue.

2 participants