Skip to content

[FLINK-2714] [Gelly] Copy triangle counting logic from EnumTrianglesOpt.java to Gelly library.#1250

Closed
ssaumitra wants to merge 3 commits intoapache:masterfrom
ssaumitra:FLINK_2714
Closed

[FLINK-2714] [Gelly] Copy triangle counting logic from EnumTrianglesOpt.java to Gelly library.#1250
ssaumitra wants to merge 3 commits intoapache:masterfrom
ssaumitra:FLINK_2714

Conversation

@ssaumitra
Copy link
Contributor

Also reorganizing classes to use Gelly's Graph APIs.

…to Gelly library. Also reorganizing classes to use Gelly's Graph APIs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just FYI, I am referring to vertices of Triad and edgesWithDegrees as V1 and V2. And vertices of edge as 0 and 1.
IMHO, org.apache.flink.graph.Edge can also have static variables V1, V2 so that group operators are more readable.

@vasia
Copy link
Contributor

vasia commented Oct 11, 2015

Hi @ssaumitra! Thanks a lot for the pull request, it looks good.
I was thinking, since this algorithm not only counts but actually enumerates all triangles, we could have 2 library methods, TriangleCounter and TriangleEnumerator. They would basically use the same algorithm internally, but the first would return the number of triangles, while the second would return a DataSet of Tuple3, containing the vertex IDs that form triangles. What do you think?

@ssaumitra
Copy link
Contributor Author

It's good idea. In my opinion, if we provide TriangleEnumerator as a library function, TriangleCounter would be trivial extension to it (map 1, reduce a+b). So would it be required to add it in library?
Also if we want to expose triangle enumerator, I think we need to make Triad as a part of library too (right now it's private class)

@vasia
Copy link
Contributor

vasia commented Oct 11, 2015

Yes @ssaumitra, if we have the enumerator, then it's a matter of calling a count to get the number of triangles. So, let's just got for TriangleEnumerator in the library. As for exposing the Triad, there's not need to do so. Triad actually extends Tuple3, so I would say to simply return DataSet<Tuple3>.

@ssaumitra
Copy link
Contributor Author

Fair enough. I will make this change in my pull request then. Can you please edit the JIRA too? So that updated pull request and JIRA title stay relevant?

@vasia
Copy link
Contributor

vasia commented Oct 11, 2015

Sure, will do! Thank you!

@ssaumitra
Copy link
Contributor Author

I have converted it to TriangleEnumerator now.

Copy link
Contributor

Choose a reason for hiding this comment

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

unused import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I was supposed to optimise them,

@vasia
Copy link
Contributor

vasia commented Oct 15, 2015

Hi @ssaumitra,
thanks a lot for updating the PR! I have left a few minor comments that should be easy to fix.

@ssaumitra
Copy link
Contributor Author

@vasia I have updated it. Please have a look. Also should I be worried about Travis build failure? Looks like it's failing on master too,

@vasia
Copy link
Contributor

vasia commented Oct 16, 2015

Thanks a lot for the quick update @ssaumitra! It looks good now :)

@vasia
Copy link
Contributor

vasia commented Oct 18, 2015

I will add a description in the docs, squash the commits, and merge this.

@asfgit asfgit closed this in d047ddb Oct 18, 2015
s1ck pushed a commit to s1ck/flink that referenced this pull request Oct 20, 2015
update test to get the directed graph as input

This closes apache#1250
cfmcgrady pushed a commit to cfmcgrady/flink that referenced this pull request Oct 23, 2015
update test to get the directed graph as input

This closes apache#1250
lofifnc pushed a commit to lofifnc/flink that referenced this pull request Oct 23, 2015
update test to get the directed graph as input

This closes apache#1250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants