Add weights to export by default

[cyberscribe]

This update adds weights using the graphml specification key/data format.

[clue]

Thanks for your PR, much appreciated 👍

Unfortunately, this currently causes the tests to fail.

See also graphp/graph#114 and #5.

[cyberscribe]

Hmm.. OK, not sure what your PHPUnit tests are aiming to assert/accomplish, but the GraphML output by this change conforms to the specification...

[clue]

Didn't mean to discourage your contribution, this PR is much appreciated 👍

The test exports the graph and then imports it again and tries to assert the imported graph looks exactly like the original one. You're (correctly) exporting the weight to a GraphML key, but upon importing it does not end up in the edge weight but instead as an edge attribute. Hence the link to the first issue, as the weight properties etc. will be removed in a future version.

Your code currently always exports the weight even if it is not set. This means the existing code always imports an empty attribute for these GraphML keys.

What do you think about this?

[cyberscribe]

What is your intent in moving to attributes in a future version? Will the getWeight/setWeight be deprecated at some point?

My suggestion would be that a) the underlying storage most closely represents what GraphML actually does (i.e. any kind of mapped key for weights) and b) that you not break backward-compatibility with existing implementations of getWeight/setWeight.

So, if you are already doing this, sounds like just the Importer needs to be changed to be symmetrical with the Exporter.

With a default value set, blank weight should in theory work. For better tidiness, it would be nice if there were a method like Graph::hasEdgeWeight() which defaults to false and only gets set when an Edge with a weight gets added. This way we could wrap all weight-related output in $graph->hasEdgeWeight(), suppressing it if unnecessary.

The packages currently do everything I need them to do (thank you) for exporting GraphML and rendering dot images, so I'd be reticent to upgrade if a future version broke backward compatibility with the current methods for manipulating weights.

[clue]

See graphp/graph#114 for why we would like to remove setWeight() (and family). Yes, this will include a BC break and a clear upgrade guide for existing code. This has been discussed in late 2014 first, as I've always tried to give people plenty of time to prepare for future BC breaks.

GraphML does support arbitrary attributes. And so do we. The above ticket will make sure we do no longer store any "special properties" but will instead consistently use our attribute store.

In this case, we can simply loop over all existing attributes for each edge and then export this to GraphML.

[cyberscribe]

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.

Again, 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.

On 14 July 2016 at 10:45, Christian Lück notifications@github.com wrote:

See graphp/graph#114 graphp/graph#114 for why we
would like to remove setWeight() (and family). Yes, this will include a
BC break and a clear upgrade guide for existing code. This has been
discussed in late 2014 first, as I've always tried to give people plenty of
time to prepare for future BC breaks.

GraphML does support arbitrary attributes. And so do we. The above ticket
will make sure we do no longer store any "special properties" but will
instead consistently use our attribute store.

In this case, we can simply loop over all existing attributes for each
edge and then export this to GraphML.

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#7 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAqV5cXIrziaC4Wc4UUAyeo9yrWhVV-Nks5qVgU7gaJpZM4JLxO0
.

[clue]

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.

I think you're raising some very valid points here. May I ask you to post this on graphp/graph#114 instead?

[cyberscribe]

Cross-posted here:
graphp/graph#114 (comment)

On 14 July 2016 at 18:56, Christian Lück notifications@github.com wrote:

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.

I think you're raising some very valid points here. May I ask you to post
this on graphp/graph#114 graphp/graph#114 instead?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#7 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAqV5fQGF2rexw0lunPxwdYWkMFMrEAyks5qVnhEgaJpZM4JLxO0
.