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

Undirected Graph Support in Cassovary #141

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

plofgren
Copy link
Contributor

Related to #140
The idea of this wrapper is that each graph implementation shouldn't need custom code for StoredGraphDir.Mutual. We can write a single wrapper which works with any graph implementation.
Here is an initial undirected graph class. If this looks good, I can add a test and support for mutable undirected graphs to this pull request.
@pankajgupta

@@ -0,0 +1,29 @@
package com.twitter.cassovary.graph
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls add the standard copyright header you can get from other files (except change 2014 to 2015)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@pankajgupta
Copy link
Contributor

@szymonm : I was also just thinking that, and agree that parametrizing Graph with NodeType will be better. Let's do that in a separate PR. See my comment above, we can do getNodeById to return UndirectedNode in UndirectedGraph regardless.

@szymonm
Copy link
Contributor

szymonm commented Jan 31, 2015

On the other hand, I used: https://github.com/scala-graph/scala-graph.

They use:

trait GraphLike[
  N,
  E[X]  <: EdgeLikeIn[X], 
  +This[X, Y[X]<:EdgeLikeIn[X]] <: GraphLike[X,Y,This] with AnySet[Param[X,Y]] with Graph[X,Y]
]

and IMO this type magic makes the library very hard to use...

So let's be carefull about that.

@pankajgupta
Copy link
Contributor

:) Agreed that there's a balance. Parametrizing by NodeType is good I think.

@plofgren
Copy link
Contributor Author

plofgren commented Feb 9, 2015

(Sorry for the delay). I changed UndirectedGraph to a trait, made UndirectedNode a sub-trait of UnidirectedNode, and made UndirectedGraph's methods return UndirectedNode. I think that addresses all of your comments. I also just rebased from master.

@pankajgupta
Copy link
Contributor

Ouch, somehow I missed your updated commits, Peter!
Also, looks your rebase has gone awry as the diff in this PR is showing other changes unrelated to your change. May I ask you to build a new patch or create a clean one on top of current master. Thanks.

@pankajgupta
Copy link
Contributor

Ping @plofgren

@plofgren
Copy link
Contributor Author

To be honest, I don't think I'll get back to this until at least after I finish my PhD in a couple months. It doesn't really add functionality, so I propose we just drop it and optionally change the documentation as suggested in #140.

@pankajgupta
Copy link
Contributor

Ah, you have done a lot of work. Let's leave this open for now and hope you come back to it when you get time. Or someone else might pick it up too.

@plofgren
Copy link
Contributor Author

Sounds good.

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 4 committers have signed the CLA.

❌ Pankaj Gupta
❌ plofgren
❌ pankajgupta
❌ szymonm


Pankaj Gupta seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants