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

Tarjan's strongly connected component and Shortest Cycle finder algorithm #106

Closed
wants to merge 19 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented May 30, 2014

@clue
Copy link
Member

clue commented May 30, 2014

Thanks a bunch, the new algorithms are very much appreciated! 👍

Given the vast amount of changes theses introduce, expect the review to take a few days. But I'll come back to this ASAP.

@ghost
Copy link
Author

ghost commented May 30, 2014

Great !


// Run the algorithm
// echo $this->graph->__toString(); die;
$algorithm = new Tarjan($this->graph);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Guess the comments above should go.

@ghost
Copy link
Author

ghost commented Jun 2, 2014

Nice feedbacks, fixed them !

* You can use Tarjan Algorithm to find such graphs.
* @see http://stackoverflow.com/questions/10456935/graph-how-to-find-minimum-directed-cycle-minimum-total-weight
*/
class CycleFinder extends BaseGraph
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this class and its tests is independent of the other changes? If so, would you care to make this a separate PR?

New algorithms are very much appreciated, keeping them separated helps reviewing the changes and will get things in a lot easier 👍

@clue
Copy link
Member

clue commented Jun 9, 2014

Finally had the time to review this. Now with all my remarks in place, let me re-emphasize that the new algorithms are very much appreciated! 👍

I hope my feedback doesn't come over as criticizing, I'm really looking forward to support in fixing the outstanding issues so we can get this in.

Thanks for your work!

@ghost
Copy link
Author

ghost commented Jun 9, 2014

No pb clue, your feedbacks are much appreciated, making me a better developer :) I'll try to find some time this week to get the work done.

@dav-m85
Copy link

dav-m85 commented Nov 30, 2014

I just removed all unwanted additions and just left the Tarjan strongly connected algorithm. I replaced VerticeDataMap class by a SPLObjectStorage. Btw, I changed my username on github, dmoreaulf RIP.

@clue
Copy link
Member

clue commented Feb 25, 2015

Sorry for the delay and thanks for your continuous interest!

All algorithms will be split off to a separate package graphp/algorithms (see also #119 for some background). As such, this PR can no longer be merged into this repo.

That being said, I'd still love this see this in! 👍 I've filed a new ticket as a reminder to keep track of this PR until it has been migrated over.

@dav-m85
Copy link

dav-m85 commented Feb 26, 2015

Migrated here graphp/algorithms#14.

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