search

mlpack / models

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

Namespace and include guard #55

Closed Aakash-kaushik closed 3 years ago

Aakash-kaushik commented 3 years ago

Implements issues discussed in #54 Please merge it only after #52 closes #54

Aakash-kaushik commented 3 years ago

I have one more proposal, I think it is better to keep the model hpp files directly inside the models folder in the repo rather than having each individual folder. this will allow the user to do something like this:

#include <models/models/darknet.hpp>

rather than

#include <models/models/darknet/darknet.hpp>

and also why is the vae folder still present in the models repo when i think i can see the same examples in the examples repo too and these ones don't directly show any capabilities of the models repo too.

Aakash-kaushik commented 3 years ago

Btw @rcurtin when you review this PR again can you also take a look at the style checks? they pass but the build always results in a failure.

zoq commented 3 years ago

Let's see if I fixed the issue.

zoq commented 3 years ago

@mlpack-jenkins test this please

Aakash-kaushik commented 3 years ago

Let's see if I fixed the issue.

Man you are magic, what did you do ? I don't know how jenkins and style checks works but would you mind explaining what the problem was, I am curious.

Aakash-kaushik commented 3 years ago

Looks like this include some changes from the merged catch transition PR. Can you merge this PR against the latest master branch.

Yes it does but i thought as soon as the changes from that catch PR are merged the files in this PR would dynamically update, but that doesn't seems to be the case.

zoq commented 3 years ago

Let's see if I fixed the issue.

Man you are magic, what did you do ? I don't know how jenkins and style checks works but would you mind explaining what the problem was, I am curious.

The Jenkins Style Check job used an outdated plugin (GitHub updated the API) to push the job status to GitHub, we already used another plugin that handles the communication with the GitHub API, so that outdated plugin isn't needed anymore.

Aakash-kaushik commented 3 years ago

Let's see if I fixed the issue.

Man you are magic, what did you do ? I don't know how jenkins and style checks works but would you mind explaining what the problem was, I am curious.

The Jenkins Style Check job used an outdated plugin (GitHub updated the API) to push the job status to GitHub, we already used another plugin that handles the communication with the GitHub API, so that outdated plugin isn't needed anymore.

Understood, thanks for taking the time to write this.

Aakash-kaushik commented 3 years ago

Looks like this include some changes from the merged catch transition PR. Can you merge this PR against the latest master branch.

Hey @zoq i did merge this agains models/master but it still shows all the same changes. I can open a new PR but it would be hard to keep track of the comments and things that ryan left.

rcurtin commented 3 years ago

From my perspective everything looks great to merge once we handle the namespace and CTest issues. :+1: Thanks for working on this @Aakash-kaushik!

Aakash-kaushik commented 3 years ago

From my perspective everything looks great to merge once we handle the namespace and CTest issues. 👍 Thanks for working on this @Aakash-kaushik!

Happy to learn and work on mlpack any day.

zoq commented 3 years ago

Looks like this include some changes from the merged catch transition PR. Can you merge this PR against the latest master branch.

Hey @zoq i did merge this agains models/master but it still shows all the same changes. I can open a new PR but it would be hard to keep track of the comments and things that ryan left.

Opening a PR might be the easiest solution, since the PR is just going to be closed afterwards the comments are not lost and the PR is pretty close to get merged anyway.

Aakash-kaushik commented 3 years ago

Looks like this include some changes from the merged catch transition PR. Can you merge this PR against the latest master branch.

Hey @zoq i did merge this agains models/master but it still shows all the same changes. I can open a new PR but it would be hard to keep track of the comments and things that ryan left.

Opening a PR might be the easiest solution, since the PR is just going to be closed afterwards the comments are not lost and the PR is pretty close to get merged anyway.

So i will close this PR when i open the new one with changes and will tag this PR in that too.

Aakash-kaushik commented 3 years ago

Closing in favour of a new PR because it carried some commits from another PR and was comparing changes the wrong way.