Closed urafi closed 8 years ago
Including the trained models is a bad idea, they are multiple GB large and that'd blow the repo out of proportions for anyone, anywhere!
So it needs a README.md
with instructions on how to run, and where to get the models from.
yes, please!
Pay attention to the meeting!
;)
you say models are a few GB. so how much RAM does it ?
I don't remember that, @urafi should answer.
I will upload a readme file containing a link to models and instructions how to use them.
@lucasb-eyer : those were the previous models and previous approach and it was 1GB. The current model is 17MB only. @marc-hanheide : The amount of RAM used was appx the same. I will still prefer to keep models separate as their size will increase in future although not in GBS.
as long as we make them downloadable. I suggest to include the download in the build process, so that the models can also be easily deployed.
seconded
ok will upload a small script that downloads the models and makes sure they can be deployed.
On Wed, Oct 14, 2015 at 4:49 PM, Marc Hanheide notifications@github.com wrote:
as long as we make them downloadable. I suggest to include the download in the build process, so that the models can also be easily deployed.
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-148073521 .
please call it from CMakeLists.txt
as part of the normal build process and then also provide install targets for it. Talking about install targets: They are missing from the current CMakeLists.txt
Ok will call it from CMakeLists.txt and also add install targets.
On Wed, Oct 14, 2015 at 5:04 PM, Marc Hanheide notifications@github.com wrote:
please call it from CMakeLists.txt as part of the normal build process and then also provide install targets for it. Talking about install targets: They are missing from the current CMakeLists.txt https://github.com/strands-project/strands_perception_people/pull/178/files#diff-3e6cfb185b4216eed7a86334c90280a4R144
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-148078158 .
oh, btw, cool this is in :+1:
Note that CMake's download function doesn't support HTTP timestamps or similar feature and will thus re-download the file on every build.
but cmake allows to create local "cookie" files and if you're clever you first download the MD5 checksum file and compare it to the local cached version :-)
True that. I have much more to say about this, but I'll stop going off-topic.
Apart from the minor comments above, more than happy to merge once the models are also included.
hi... this had been open for a bit now... can I have an ETA for the changes requested please?
I am waiting to get new training data from LEEDs to update the models and then upload them. You can close it now. I will generate a new request when I have new trained models.
On Wed, Dec 2, 2015 at 7:58 AM, Marc Hanheide notifications@github.com wrote:
hi... this had been open for a bit now... can I have an ETA for the changes requested please?
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-161202014 .
I think we should keep this open and you just keep adding the new models to this PR, ok?
works for me.
On Wed, Dec 2, 2015 at 12:03 PM, Marc Hanheide notifications@github.com wrote:
I think we should keep this open and you just keep adding the new models to this PR, ok?
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-161261332 .
Any update on this?
I just got data from Leeds guys. I am working on retraining the models for skeleton detector. It will take some time. I will update when I have the new models ready.
On Wed, Jan 13, 2016 at 5:51 PM, Marc Hanheide notifications@github.com wrote:
Any update on this?
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-171361501 .
Hi Marc. I have added Model (since it 451 kb only), install targets and README file. Please have a look and let me know if anything else needs to be rectified.
On Wed, Jan 13, 2016 at 5:51 PM, Marc Hanheide notifications@github.com wrote:
Any update on this?
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-171361501 .
Thanks, please have a look at the other issues raised in individual comments by @cdondrup and myself.
Thanks
Are there install targets for the models?
Yes. Thank you for pointing this out. Just fixed it. Thanks a lot @marc-hanheide and @cdondrup for the feedback.
@urafi Thanks for all the changes! Apart from the one minor one above, I think this is fine now.
@urafi for the future, please use more descriptive commit messages than "modified file". These messages go into the packages ChangeLogs, so they should describe what you did.
And, for install targets, it's best practice to run catkin_make install
in your workspace, and then try to use the installed version to test it, e.g. run source ./install/setup.bash
instead of source ./devel/setup.bash
and then use your launch file. Then you can make sure that all files are installed where you would expect them and that the package will also contain all it should.
fixed the version number. @marc-hanheide Thanks for the tip for checking installed package.
Let's see how this works out when I release it ;-)
not very optimistic :)
On Wed, Feb 17, 2016 at 3:40 PM, Marc Hanheide notifications@github.com wrote:
Let's see how this works out when I release it ;-)
— Reply to this email directly or view it on GitHub https://github.com/strands-project/strands_perception_people/pull/178#issuecomment-185233280 .
congratulations... All went smoothly: https://lcas.lincoln.ac.uk/jenkins/job/ros-indigo-rwth-upper-body-skeleton-random-walk_binarydeb_trusty_amd64/
anyone fancies trying it out with sudo apt-get install ros-indigo-rwth-upper-body-skeleton-random-walk
?
This might be of interest to @OMARI1988, @hawesie and @agcohn as well: We have a first release of the RWTH upper body tracking.
Would you also include the trained models? And write a small
README.md
to tell people how to use it, please?