Open rcurtin opened 4 years ago
Thanks @rcurtin for opening this issue. Maybe it makes sense to remove one example at a time and replace it with the new model because when I made the changes earlier for all examples I had added 1700 lines and removed about 6k lines which would be difficult to review, so I'll open a PR tomorrow to replace mnist portion of the repo. I'll also try to write cmake such that it matches for git submodules. Once that is done, I think it will be easier to make changes for the rest. I can do them in 1 PR also if that makes more sense. Thanks a lot.
Agreed, sounds good. Getting the submodule part set up might require a little bit more work. (@shrit maybe you have some input on the best way to do that?) Basically, if we use submodules, it would probably mean that users can't build from this repository directly. I guess that's okay?
I opened a PR to update the readme in #2.
@rcurtin The good news is that we can build from the repository itself, even if we add it as a submodule (Assuming as before that mlpack and all other dependencies are installed correctly). In all cases, we need to have a cmake build system for the models, which is going to be used by mlpack cmake to build all the models if a user wants. I can work on a pull request on mlpack to add this repository as a submodule and compile it directly from mlpack.
@shrit that sounds good, we can try that and see how it works. It might be good to wait until we finish transitioning this repository so that it has a bunch of bindings that use the CLI framework in it though? Either way is fine I guess. :)
@rcurtin Yes, I am able to integrate models into mlpack
. The main issue so far is the compilation order (models
are compiling before mlpack
). Since models
cmake system is going to be modified in all cases. I think I am going to wait until the model repository converge
@shrit take a look at the end of src/mlpack/CMakeLists.txt
, it could cause some problems. Basically, the if (BUILDING_PYTHON_BINDINGS)
section and post_markdown_setup()
need to be called after all bindings are added. But we would have to add all models/
bindings before calling this code. So some restructuring of the core mlpack CMake code might be necessary (that's not a problem though).
It looks like the item remove any "simple" examples and replace with bindings that have CLI support, etc.
is being handled by #3. Thanks @kartikdutt18!
I'll update the website and references to this repository within the next day.
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:
I just wanted to create an issue to point out that there are some cleanup bits that need to be done in this repository after the changes we decided to make to the repository structure in mlpack/examples#61.
This is the "models" repository, which will contain ready-to-use implementations of popular machine learning model types that aren't available in the normal mlpack codebase. These can be used as bindings.
So, here's a basic todo: