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

Nits on NSGA2 #262

Merged
merged 3 commits into from
Mar 5, 2021
Merged

Nits on NSGA2 #262

merged 3 commits into from
Mar 5, 2021

Conversation

jonpsy
Copy link
Member

@jonpsy jonpsy commented Mar 3, 2021

Hi @say4n , I've been reading your code on NSGA2 and I really like it :). I've made some small corrections.

  • minor grammar fix
  • replace std::endl with "\n"

- remove std::endl
@mlpack-bot
Copy link

mlpack-bot bot commented Mar 3, 2021

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Tests that are added follow the testing guide
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

@ShikharJ
Copy link
Member

ShikharJ commented Mar 3, 2021

Replacing std::endl with \n is a useless change.

@jonpsy
Copy link
Member Author

jonpsy commented Mar 3, 2021

In hindsight, yes, but I'd rather avoid them if possible. Anycase, these changes are minor nitpicks.

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Hey @jonpsy, thanks for the comment fix! 👍 mlpack and ensmallen code tend to use std::endl throughout, so we probably ought to stick with that.

Did you have more changes you were planning to make, or is this ready to go? (If you plan on changing more, we can wait until you're done to finish the review process. :))

include/ensmallen_bits/nsga2/nsga2_impl.hpp Outdated Show resolved Hide resolved
Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Nice catch, even if that one is a small fix, I think it's important to have correct and clear comments 👍

include/ensmallen_bits/nsga2/nsga2_impl.hpp Outdated Show resolved Hide resolved
@jonpsy
Copy link
Member Author

jonpsy commented Mar 4, 2021

Thanks for the reviews. The primary goal of this PR was to get acquainted to the reviewing process. I was making the first pass at this code so I caught a few nitpicks which I've posted.I do realize my changes were not substantial.

I've made some more changes, but I'd like your opinions before I make them (more than just grammar fix :) ). Can we do it on a seperate PR? Can we merge this one for the time being?

I'd like this code to be

  • Minimize use of for loops
  • Write cache-friendly code
  • Reduce code length
  • Possibly structure it to inherit from abstract class "MultiObjective" so that it gives space to add MOEA/D (my next work)

@jonpsy
Copy link
Member Author

jonpsy commented Mar 4, 2021

There's also a prospect of parallelizing through OMP.

@jonpsy jonpsy mentioned this pull request Mar 4, 2021
10 tasks
@zoq
Copy link
Member

zoq commented Mar 4, 2021

Thanks for the reviews. The primary goal of this PR was to get acquainted to the reviewing process. I was making the first pass at this code so I caught a few nitpicks which I've posted.I do realize my changes were not substantial.

I've made some more changes, but I'd like your opinions before I make them (more than just grammar fix :) ). Can we do it on a seperate PR? Can we merge this one for the time being?

I'd like this code to be

  • Minimize use of for loops
  • Write cache-friendly code
  • Reduce code length
  • Possibly structure it to inherit from abstract class "MultiObjective" so that it gives space to add MOEA/D (my next work)

Let's continue the discussion in #263.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@mlpack-bot
Copy link

mlpack-bot bot commented Mar 5, 2021

Hello there! Thanks for your contribution. I see that this is your first contribution to mlpack. If you'd like to add your name to the list of contributors in COPYRIGHT.txt and you haven't already, please feel free to push a change to this PR---or, if it gets merged before you can, feel free to open another PR.

In addition, if you'd like some stickers to put on your laptop, I'd be happy to help get them in the mail for you. Just send an email with your physical mailing address to [email protected], and then one of the mlpack maintainers will put some stickers in an envelope for you. It may take a few weeks to get them, depending on your location. 👍

@@ -75,7 +75,7 @@ typename MatType::elem_type NSGA2::Optimize(
if (lowerBound.n_rows == 1)
lowerBound = lowerBound(0, 0) * arma::ones(iterate.n_rows, iterate.n_cols);

// Check if lower bound is a vector of a single dimension.
// Check if upper bound is a vector of a single dimension.
Copy link
Member

Choose a reason for hiding this comment

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

Oops, thanks, great catch! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@say4n, Hi, thanks. I'd love your reviews on the follow-up PR. Cheers! :)

@zoq zoq merged commit 28a37d8 into mlpack:master Mar 5, 2021
@zoq
Copy link
Member

zoq commented Mar 5, 2021

Thanks for the contribution.

@jonpsy jonpsy deleted the minor branch March 5, 2021 22:09
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.

5 participants