Fix nsga2 bug

[ewu63]

Purpose

Closes #340 (hopefully)...

Summary of changes:

• Fixed some pointer allocations (from @eirikurj based on this comment
• I traced down a bug when the population size is not a multiple of 4. Not sure if what I did here is correct or not, the easy way would be to just force population to be a multiple of 4 (as done here)
• It seems that NSGA2 cannot be called twice in sequence. The single and 2-objective tests pass individually, but when run together a segfault happens. Running with -i via testflo causes the tests to pass locally. I did take a quick look with Valgrind but it seems that the C code is just super messy and I guess I'm not surprised it's not reentrant.
• Seems that the Windows tests are still flakey even with -i. If this keeps happening we can choose to disable those tests again I suppose..

Expected time until merged

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.97%. Comparing base (6c877b4) to head (2459911).
Report is 4 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #438       +/-   ##
===========================================
+ Coverage   75.18%   85.97%   +10.78%     
===========================================
  Files          22       22               
  Lines        3313     3315        +2     
===========================================
+ Hits         2491     2850      +359     
+ Misses        822      465      -357     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcomangano]

Looks like the segfault is showing up again

[ewu63]

Pushed some changes and updated the original post, but seems that some tests are still possibly flaky..

[marcomangano]

Thanks for the fixes. I think we could merge this and see if the failures still show up as we work on #435 . Fine with skipping Windows tests if they turn out to be the problem.

[ewu63]

Bump @eirikurj

[eirikurj]

Looks good, see my one comment for now.

[eirikurj]

Allowing any popsize the previous implementation does look like a bug indeed, and it will run past, e.g., popsize=5 will try to run through 4,5,6,7 so out of bounds. However, the current patch I think is not fully sufficient though since depending on popsize it might skip the last four e.g., popsize=8, or if popsize=4 it will just not run at all. I think the better solution is the one you suggested that is enforcing popsize in increments of four. This requires the least amount of code changes and avoids these corner cases. If we want to allow any popsize, then this logic and code needs to be improved further. In its current state though, we will need to skip some regardless, which makes allocating a larger popsize useless and not worth it.
I think we should revert this and just enforce popsize % 4 == 0 as this will also catch the popsize<4 case. I can push some changes. Feel free to change or update as needed.

[ewu63]

@eirikurj agree with what you said, change looks good. Since I can't approve my own PR please approve/merge. Thanks!