eldar / pose-tensorflow

Human Pose estimation with TensorFlow framework
GNU Lesser General Public License v3.0
1.14k stars 384 forks source link

[Suggestion] make this module importable in Python; control over GPU options #87

Open cxrodgers opened 6 years ago

cxrodgers commented 6 years ago

Hi @eldar thank you for this amazing work! I am using this algorithm to track animal pose in images and it works fantastically.

I have forked this repository and made a few simple interface changes to make it a little easier to use in a Python script, especially in a multi-GPU server environment. https://github.com/cxrodgers/PoseTF

So for example, you can now do:

import PoseTF
PoseTF.training.train(cfg_file=path_to_config_file, memfrac=.5, choose_gpu=3)

to train a model in an arbitrary directory, using 1/2 the memory on GPU 3.

All of the other functions in this repository are also available in the PoseTF module. (I had to rename without a hyphen to make it a valid module name.)

I'm not sure whether you are interested in these changes, but if you are, I could create a PR and you could pull into a new branch. In any case, just thought it might be useful for someone who is using it.

Thanks!

eldar commented 6 years ago

Hi Chris!

Thanks for your interest, I'm glad it served you well! It seems like a pretty big change and I would like to look closer at it. I don't have experience with releasing python packages as a library, I will need to look at the guidelines, to see how it is usually done. It seems like a good idea though.

A few comments/questions. It seems that that you rely on the fact that repository name serves as the name of the package, hence it will have to be renamed, and I don't feel like it is necessary. The approach I would take is to move all the code files to the PoseTF (or some other name) subfolder. That would make it cleaner, but needs some work in order to update documentation, paths etc. I would expect that the way imports are called will have to be changed too.

You are now setting the environment variable CUDNN_USE_AUTOTUNE programmatically, that's great actually. However I saw that the name changed from the TF_CUDNN_USE_AUTOTUNE to CUDNN_USE_AUTOTUNE. Is it intentional?

Cheers, Eldar.

cxrodgers commented 6 years ago

Hi Eldar

Re TF_CUDNN_USE_AUTOTUNE, you are right, that was a typo, thanks! This must mean that this variable has no effect on my system because I never had any problems with the misspelled variable. I fixed it.

Re the repository name: I see your points and to be clear I'm not an expert in this issue, so I may not have done it optimally. Just to explain my reasoning, I went down this path simply because import pose-tensorflow is not valid Python with the hyphen. I didn't want to import each subfolder individually, like "import nnet" and "import config", because some of those names might conflict with other packages on the system, so they need to be placed in some higher-level package namespace. Now that you mention it, I think it could work well to keep the repository name the same, and to put all the importable code in a subfolder called PoseTF.

I was able to get it working with minimal changes by using relative imports, so from dataset.pose_dataset import PoseDataset, Batch becomes from .pose_dataset import PoseDataset, Batch and so on. Because these are relative, it might be easy to move everything into a new subfolder PoseTF as you suggested.

I understand it's a big change and it may not make sense for you to go through the work of accepting it. Also, I'm not using a lot of the functionality, and in fact I commented out the mscoco imports because I don't have pycocotools installed. So, it would not make sense to merge this PR into your master branch without more testing and improvements first. How we proceed probably depends on how many people are interested in these changes.