search

hassony2 / torch_videovision

Transforms for video datasets in pytorch
GNU General Public License v3.0
269 stars 44 forks source link

Packaging the code #6

Closed bachsh closed 4 years ago

bachsh commented 4 years ago

Hey,

I know it's been a while since you're written the code but I found it very helpful and it was more convenient to package it for installation. It's very basic but does the trick. After merging, you need to update the installation part in the README to point to your repo.

If you want, you can also submit the code to PyPI after merging. I would've done so myself but it's your code so naturally it's your call to make.

hassony2 commented 4 years ago

Thank you for the pull request @bachsh, this is useful ! I will make a quick sanity check this week, and then merge it. Thank you also for removing the useless imports :)

Best,

Yana

bachsh commented 4 years ago

Cool! After you merge don't forget to change the URL in the README.

Removal of useless imports is thanks to PyCharm ❤️

bachsh commented 4 years ago

Hey Yana,

I fixed an issue with internal imports of the utils module. I haven't noticed it before because I only used the video_transforms module, and tested the package from inside the project. I now tested the package externally and the imports work properly to my understanding. Apologies

FabianHertwig commented 4 years ago

I basically created the same pull request and just then saw that there is already one. At least I can review this pull request.

In the funtional.py file is an import of cv2. So should opencv-python be in the install_requires part of the setup.py? https://github.com/bachsh/torch_videovision/blob/master/torchvideotransforms/functional.py https://github.com/bachsh/torch_videovision/blob/master/setup.py

Cheers, Fabian

bachsh commented 4 years ago

Thanks for the review!

Yes, it should, but in my experience I wasn't able to install opencv-python using pip. It should be installed using the system package manager (apt, yum, brew etc.). I added this step to the installation part in the README. If you were able to install it using pip please update accordingly.

Avi

On Thu, Feb 6, 2020 at 1:26 PM Fabian notifications@github.com wrote:

I basically created the same pull request and just then saw that there is already one. At least I can review this pull request.

In the funtional.py file is an import of cv2. So should opencv-python be in the install_requires part of the setup.py?

https://github.com/bachsh/torch_videovision/blob/master/torchvideotransforms/functional.py https://github.com/bachsh/torch_videovision/blob/master/setup.py

Cheers, Fabian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hassony2/torch_videovision/pull/6?email_source=notifications&email_token=ABRRDCBADPNFTAE7EE64WZDRBPXX7A5CNFSM4KFUJUR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK64CTI#issuecomment-582861133, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRRDCHWOB3AMA6EKFU47GTRBPXX7ANCNFSM4KFUJURQ .