Namespace and include guard #55
Conversation
merging models/master
update models with mlpack/models
|
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. |
There was a problem hiding this comment.
Very nice work @Aakash-kaushik! I have a few minor comments. 👍
| @@ -47,7 +47,8 @@ | |||
| #include <mlpack/methods/ann/init_rules/glorot_init.hpp> | |||
|
|
|||
| namespace mlpack { | |||
| namespace ann /** Artificial Neural Network. */{ | |||
| namespace ann /* Artificial neural networks */{ | |||
| namespace models { | |||
There was a problem hiding this comment.
Personally I think it might be cleaner to put the various networks in mlpack::models, instead of mlpack::ann::models. Let me know what you think. 👍
There was a problem hiding this comment.
So just having mlpack::models was creating a issue that i cannot just use the ann namespace inside the namespace by doing the usual using namespace mlpack::ann and if i can't do that i have to go everywhere and use the the whole naming convention like mlpack::ann::component_i_want_to_use so to avoid huge code changes I used the ann namespace too.
Is there a workaround?
There was a problem hiding this comment.
I think that just ann::component_i_want_to_use should work too, since both are under the mlpack namespace. But still, I see your point. The main reason I suggested mlpack::models instead of mlpack::ann::models is that someday we might add models to this repository that are not ANN models, and so the mlpack::ann::models namespace might not make sense. (Of course, in that case, we could just use mlpack::models there, perhaps?)
I can see advantages and disadvantages to both approaches, but I'm not sure which is better. From a user perspective, mlpack::models is probably easiest and clearest, but from a developer perspective, the disadvantage is that we may need ann:: in the code.
What do you think? Also, @kartikdutt18, @zoq, and others, any opinions or thoughts? I'm pretty on the fence on this one; no strong opinion.
There was a problem hiding this comment.
I am on the side of mlpack::model because of these points
- There could be more models that are not ANN
- Easy for user
Let me know what others think and i can make the changes.
There was a problem hiding this comment.
Agreed, the plan was always to use this for everything mlpack implements, the ann part is only a small part, so I think it makes sense if the namespace reflects that.
There was a problem hiding this comment.
i'll start making the changes.
| @@ -30,32 +30,7 @@ target_link_libraries(models_test | |||
| # Get the list of sources from the test target. | |||
| get_target_property(test_sources models_test SOURCES) | |||
|
|
|||
| # Go through the list of test sources and parse the test suite name. | |||
There was a problem hiding this comment.
Should we really remove this? If we transitioned to using catch, then I think we can regex on TEST_CASE(..., <test suite name>) instead of BOOST_AUTO_TEST_SUITE and it should still work fine. It might take a little bit of work to adapt it.
There was a problem hiding this comment.
I mean yes we can adapt it to catch but i don't know what purpose it serves ?
There was a problem hiding this comment.
Essentially this part of the code allows CTest integration for each model type. So, with this code, we have the ability to have one CTest test suite for each model type, which is nice. Otherwise, if you used CTest, it groups everything as one test suite, but especially for these models, tests for a model type may take a long time and so it might be preferable to have the ability to run only one test at a time.
There was a problem hiding this comment.
Makes absolute sense, I will implement it.
|
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. |
|
Let's see if I fixed the issue. |
|
@mlpack-jenkins test this please |
Man you are magic, what did you do ? |
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. |
* Update windows-steps.yaml * Update windows-steps.yaml * copying dll and lib files to bin * copy dll and lib to build/test * copy dll and lib to build/test * cleanup * added exclusion for catch * style check solve * fix style check * trigger build Co-authored-by: kartikdutt18 <39593019+kartikdutt18@users.noreply.github.com>
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. |
Hey @zoq i did merge this agains models/master but it still shows all the same changes. |
|
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. |
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. |
|
Closing in favour of a new PR because it carried some commits from another PR and was comparing changes the wrong way. |
Implements issues discussed in #54
Please merge it only after #52
closes #54