Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preparation for Packaging #5

Merged
merged 2 commits into from
Feb 16, 2021
Merged

Preparation for Packaging #5

merged 2 commits into from
Feb 16, 2021

Conversation

Sparkier
Copy link
Owner

Describe your improvements
Started packaging process by changing the project structure and defining the setup configuration.

To Reproduce
Follow the steps outlined in the README.md.

@Sparkier
Copy link
Owner Author

Hey @fuzhanrahmanian, I started this PR to make packaging of this project possible. I noticed that currently, the alexnet.py uses luna/pretrained_models/data/meta_clsloc.mat, which is not included in the project. Should we include this or can we maybe get rid of this requirement?

@Sparkier
Copy link
Owner Author

Sparkier commented Feb 16, 2021

I removed unused code from the alexnet file, so that the .mat file is not needed anymore. However, in googlenet.py, we still have:

googlenet.load_weights(os.path.join(
os.path.dirname(__file__), 'googlenet_weights.h5'))

and
return alexnet.alex_net("alexnet_weights.h5")

, which are referring to static files. We should think about how to remove that in a future PR.

Removed code for alexnet that was not used, and removed the weight
specification of googlenet.
install_requires =
tensorflow
tf_slim
keras
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is numpy not needed? (image.py uses it)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think numpy is a dependency of TensorFlow, which is why it still works I assume. However, if we use it directly we should add it. I will do that.

Copy link
Collaborator

@fuzhanrahmanian fuzhanrahmanian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except "NumPy" in the install requirements, the changes look good, thanks!

@fuzhanrahmanian fuzhanrahmanian merged commit 71ef186 into master Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants