search

mlpack / models

models built with mlpack
https://models.mlpack.org/docs
BSD 3-Clause "New" or "Revised" License
35 stars 40 forks source link

Namespace and Include guards #54

Closed Aakash-kaushik closed 3 years ago

Aakash-kaushik commented 3 years ago

Points to Discuss:

  1. For now the models repo uses mlpack namespace in some files and just plain nothing in some so should this repository have its namespace in all the files or just carry the mlpack namespace.
  2. the include guard in all the files follows a format as MODELS_FILENAME.HPP, I propose that it follows a similar structure such as mlpack which will be MODELS_FOLDER_NAME_FILENAME.HPP.

also please keep in mind that for now this repository doesn't have any installation candidate or place that it is installed in but that will be included in it very soon, I am just figuring out how to configure install with cmake and i will do that then.

rcurtin commented 3 years ago

Definitely agreed on both points! Thanks for bringing it up. Personally I think it would be fine to put things here in the mlpack::models namespace, but whatever we choose is fine. (It should definitely be in a namespace.)

Good point on the include guards too---I agree with your proposal. :+1:

Aakash-kaushik commented 3 years ago

Great, I think mlpack::models is fine and i will open a PR soon to include both of these changes.

mlpack-bot[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! :+1: