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

Add NSGA2 #149

Merged
merged 126 commits into from Aug 10, 2020
Merged

Add NSGA2 #149

merged 126 commits into from Aug 10, 2020

Conversation

@say4n
Copy link
Member

@say4n say4n commented Dec 22, 2019

This PR implements the Non-dominated Sorting Genetic Algorithm - II by Kalyanmoy Deb.
It is a Work in Progress. :)

TODO:

  • Implement NSGA-II
  • Add tests
  • Add documentation
say4n added 8 commits Dec 24, 2019
tests/nsga2_test.cpp Outdated Show resolved Hide resolved
say4n added 3 commits Dec 26, 2019
@say4n say4n requested a review from favre49 Dec 28, 2019
@say4n say4n changed the title Add NSGA2 (WIP) Add NSGA2 Dec 28, 2019
Copy link
Member

@favre49 favre49 left a comment

I've only made a quick read of it, I'm currently on vacation so I didn't get much time to read it deeply. In the meantime, could you document the code too? That could aid the reviewers in understanding what your code is doing and make this process quicker :)

include/ensmallen.hpp Outdated Show resolved Hide resolved
include/ensmallen_bits/nsga2/nsga2.hpp Outdated Show resolved Hide resolved
include/ensmallen_bits/nsga2/nsga2.hpp Outdated Show resolved Hide resolved
include/ensmallen_bits/nsga2/nsga2.hpp Show resolved Hide resolved
include/ensmallen_bits/nsga2/nsga2_impl.hpp Outdated Show resolved Hide resolved
include/ensmallen_bits/nsga2/nsga2_impl.hpp Outdated Show resolved Hide resolved
include/ensmallen_bits/nsga2/nsga2_impl.hpp Outdated Show resolved Hide resolved
say4n added 3 commits Jan 2, 2020
…ze; Correct indentation
@say4n
Copy link
Member Author

@say4n say4n commented Jan 2, 2020

In the meantime, could you document the code too?

Yes, will do it ASAP! :)

@say4n say4n requested a review from favre49 Jan 2, 2020
@rcurtin
Copy link
Member

@rcurtin rcurtin commented Jul 31, 2020

I installed valgrind on bigglesworth. 👍 Thanks so much @say4n, great work. I'll approve once I see it pass all the builds.

@say4n
Copy link
Member Author

@say4n say4n commented Aug 1, 2020

@rcurtin there are some issues with the memcheck CI job. Marcus said he'll look into it (on IRC).

The remainder of the CI jobs are green.

@zoq
Copy link
Member

@zoq zoq commented Aug 2, 2020

Valgrind over all tests:

real    553m53.868s
user    553m21.108s
sys     0m23.900s
<?xml version="1.0"?>

<valgrindoutput>

<protocolversion>4</protocolversion>
<protocoltool>memcheck</protocoltool>

<preamble>
  <line>Memcheck, a memory error detector</line>
  <line>Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.</line>
  <line>Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info</line>
  <line>Command: build/ensmallen_tests -s</line>
</preamble>

<pid>1819</pid>
<ppid>1815</ppid>
<tool>memcheck</tool>

<args>
  <vargv>
    <exe>/usr/bin/valgrind.bin</exe>
    <arg>--tool=memcheck</arg>
    <arg>--leak-check=full</arg>
    <arg>--track-origins=yes</arg>
    <arg>--num-callers=40</arg>
    <arg>--error-exitcode=1</arg>
    <arg>--xml=yes</arg>
    <arg>--xml-file=reports/tests/ensmallen.xml</arg>
    <arg>--log-file=reports/tests/ensmallen.memcheck</arg>
    <arg>--verbose</arg>
  </vargv>
  <argv>
    <exe>build/ensmallen_tests</exe>
    <arg>-s</arg>
  </argv>
</args>

<status>
  <state>RUNNING</state>
  <time>00:00:00:00.141 </time>
</status>


<status>
  <state>FINISHED</state>
  <time>00:09:13:53.384 </time>
</status>

<errorcounts>
</errorcounts>

<suppcounts>
</suppcounts>

</valgrindoutput>

Looking at the runtime, I'm going to modify the mem-check job to run only related tests.

@say4n
Copy link
Member Author

@say4n say4n commented Aug 2, 2020

Whoa ~10 hours!

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 5, 2020

Hey @favre49, do you want to take another pass at a review on this? I think it's basically ready. 👍

@favre49
favre49 approved these changes Aug 5, 2020
Copy link
Member

@favre49 favre49 left a comment

Glad to see this completed, no comments from my side. Great work! :)

@favre49
Copy link
Member

@favre49 favre49 commented Aug 5, 2020

I think once we merge this we can close #120, since this basically invalidates it. If I get the time again I can reopen it to conform to this interface and such.

@rcurtin
rcurtin approved these changes Aug 5, 2020
Copy link
Member

@rcurtin rcurtin left a comment

@favre49 agreed, that sounds good. If you like, maybe it is easy to adapt some small parts of that PR.

I guess we should wait to merge this one until the memory check job is resolved?

@zoq
Copy link
Member

@zoq zoq commented Aug 6, 2020

I can put some time to resolve the memory job runtime issue, unless @say4n already is close to push a patch?

@say4n
Copy link
Member Author

@say4n say4n commented Aug 6, 2020

@zoq I haven't yet started working on it, I was planning to do it in the weekend ahead. 😛

@zoq
Copy link
Member

@zoq zoq commented Aug 6, 2020

Okay, I went ahead and put something together: mlpack/jenkins-conf#23

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 8, 2020

Awesome, mlpack/jenkins-conf#23 is merged now. I think maybe some Jenkins configuration changes might still be needed before re-running though?

@zoq
Copy link
Member

@zoq zoq commented Aug 9, 2020

Okay, I think I have everything set up correctly now.

@zoq
Copy link
Member

@zoq zoq commented Aug 9, 2020

@mlpack-jenkins test this please

@say4n
Copy link
Member Author

@say4n say4n commented Aug 9, 2020

Whoa, the memory checks ran super fast!

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 9, 2020

Looks like maybe it didn't run any tests?

<testsuite name="ensmallen_tests" errors="0" failures="0" tests="0" hostname="tbd" time="0.141205" timestamp="2020-08-09T21:24:36Z">

I might have read the output wrong though.

@zoq
Copy link
Member

@zoq zoq commented Aug 9, 2020

@mlpack-jenkins test this please

@zoq
Copy link
Member

@zoq zoq commented Aug 9, 2020

Looks like maybe it didn't run any tests?

<testsuite name="ensmallen_tests" errors="0" failures="0" tests="0" hostname="tbd" time="0.141205" timestamp="2020-08-09T21:24:36Z">

I might have read the output wrong though.

No, let's see if I got it right this time.

@say4n
Copy link
Member Author

@say4n say4n commented Aug 10, 2020

So much green. :)

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 10, 2020

Awesome! It looks good to me and the memory check scripts seem to work fine. I'll go ahead and merge it.

@say4n do you want to help with the release? If you're willing, try this:

  • check out the ensmallen master branch
  • from the root of the repository, run scripts/ensmallen-release.sh say4n 2 14 0 "<name>" where <name> is whatever you think the next release name should be :)

I'm not sure it will work completely, but if it does, then it will automatically open a pull request using the hub tool for the next release. Anyway, I can also do it, just let me know.

@rcurtin rcurtin merged commit c73c92a into mlpack:master Aug 10, 2020
5 checks passed
5 checks passed
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mlpack master build test Build finished.
Details
@say4n
Copy link
Member Author

@say4n say4n commented Aug 10, 2020

Sure, I can create the PR for a new release.

Do we keep it Automatically Automated Automation? If not, does the name need to pertain to a certain theme?

@say4n say4n deleted the say4n:nsga2 branch Aug 10, 2020
@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 10, 2020

Nah, for a new minor release, feel free to pick a new name. :)

@say4n
Copy link
Member Author

@say4n say4n commented Aug 10, 2020

Sure, on it. :P

@rcurtin
Copy link
Member

@rcurtin rcurtin commented Aug 10, 2020

If you have any problems just let me know---the script is still a work in progress. 😄

@say4n
Copy link
Member Author

@say4n say4n commented Aug 10, 2020

Ah, so I've opened #214 but the changelog in the description for the PR is was wrong (I've corrected it now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.