Adding AGEMOEA and IGD #399
Conversation
|
Hey I plan on changing a bit more of the declarations for the current functions and plan to resolve the review changes and push the changed declarations together |
|
Sounds good to me, thanks for the update. |
|
Hey I have validated its logic with some of the existing problems in mlpack (ZDT suite) and will now proceed to do it with Maf and will put up the PR for the maf problems withing this week. If possible can you review #397 @zoq . Please feel free to also go through the code changes in this PR for any other issues. |
|
Approved #397 and it should be merged in the next 24 hours. |
|
Let me know if this is ready for review. |
|
@zoq Can you please go through this PR and suggest any possible changes. |
| //! Check if a candidate Pareto dominates another candidate. | ||
| template<typename MatType> | ||
| inline bool AGEMOEA::Dominates( | ||
| std::vector<arma::Col<typename MatType::elem_type> >& calculatedObjectives, |
There was a problem hiding this comment.
Why not just use ColType as a template parameter directly in this function and other functions? It seems very awkward to do it like this because MatType is not actually used in these functions (at least I did not see any usage at a quick glance).
There was a problem hiding this comment.
I also felt the same but decided to keep it there since a similar pattern was followed nsga2 so I kept agmoea similar to it aiming for uniform implementation as agemoea is very similar instructure to nsga2.
There was a problem hiding this comment.
It's a bit of a strange pattern, personally I think it would be a good idea to change it (it's clearer, and the way it currently is doesn't seem to provide any benefit). Could be worth changing in NSGA2 too.
There was a problem hiding this comment.
Yes, I think its better if both are done together so I can pick it up after NSGA3 has been Implemented (Using the ColType ?) and then I can work on these two
|
Sounds good. The only comments that matter to me before approval are:
Sorry to hold things up a bit on this PR. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@rcurtin I have made the requested changes. The documentation for Indicators I have added to |
There was a problem hiding this comment.
Thanks for addressing everything @IWNMWE! Sorry that there is so much trickiness and pickiness over documentation; it's what users see, so ensuring that everything is consistent everywhere is important. I left a few more suggestions, but they are only suggestions---don't feel obligated to take them. Nothing else from my side, thanks again for the hard work on this. 🚀
|
I have made all the required changes except the ColType which I can do the end of august for NSGA2 and AGEMOEA. Other than that I think this PR is good to be merged. Please let me know if there are other changes I have to make @zoq |
This PR implements the IGD score and AGEMOE optimizer.
Colab Notebook link(IGD test) : https://colab.research.google.com/drive/1fHLPFbOZCmK4Dfwqsw5Ey08lqwRDp9QK?usp=sharing