-
Notifications
You must be signed in to change notification settings - Fork 17
Added Greedy Matching Algorithm Implementation #52
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
Conversation
|
Thank you. |
|
Hello, thanks for the PR! If I understood correctly, this is a greedy algorithm for producing a minimal spanning tree, right? I think that it will be better if we rename the classes because people can get confused. We can have different types of greedy algorithms, as a greedy algorithm is an algorithm that takes the best decision at each step |
|
Hello, thanks for the answer. No, it is not about a minimal spanning tree but about matching as explained here: https://en.wikipedia.org/wiki/Matching_(graph_theory) As far as I could see, matching is the usual term mostly used in graph theory. Alternative name: independent edge set. So in my opinion, the name AIGreedyMatching is adequate, the more so as the class comment explains the concept. The name AIGreedyIndependentEdgeSet may be too heavy but acceptable. |
|
I see, I got confused too! I propose to name it AIGraphMatchingAlgorithm; what do you think? We can put in the class comment that the current implementation uses a greedy algorithm technique. I would not like to put "Greedy" in the name as it can mean several things and we have several different algos |
|
I respect the reasons for not using the term greedy in the class name, although some graph library does so. So I would definitely use the proposed name AIGraphMatchingAlgorithm, which is pretty comprehensive. Maybe we could rename it in the future still to AIGraphMatchingGreedyAlgorithm if other more elaborated graph matching algorithms are added. I also would rename the category Matching into Graph Matching, since matching is a quite general term. I am going to prepare the next draft including the renamings and modified comments. |
|
Tx Driolar this is super nice to have people improving. I'm quite lame with Graph. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Renamed classes into AIGraphMatchingAlgorithm and AIGraphMatchingAlgorithmTest as discussed.
- Renamed category "Matching" into "Graph Matching" as proposed.
- Improved comments.
- Minor improvements in a few methods.
- Added a single test.
|
Hello, thanks for the fixes! and for the discussion I will integrate this PR.
I totally agree with this. If we add more matching algos we can do the renaming |
Description
This PR contributes with the implementation of the Greedy Matching Algorithm.
The algorithm class can be instantiated for
Changes
AIGreedyMatchingclass with Greedy Matching Algorithm implementation.AIGreedyMatchingTestclass with tests.