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

Add streamlit readme #168

Merged
merged 24 commits into from
Jul 12, 2020
Merged

Add streamlit readme #168

merged 24 commits into from
Jul 12, 2020

Conversation

oke-aditya
Copy link
Contributor

Adds streamlit to readme.
Please check if the link works.

oke-aditya added 23 commits June 9, 2020 22:37
@oke-aditya
Copy link
Contributor Author

Oh yeah, closes #107. Helps in #99 #117.

@oke-aditya
Copy link
Contributor Author

Helps in #135 #39 #83

Copy link
Collaborator

@ai-fast-track ai-fast-track left a comment

Choose a reason for hiding this comment

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

I added a suggestion concerning streamlit installation

@lgvaz
Copy link
Collaborator

lgvaz commented Jul 11, 2020

Very nicee!!!

@oke-aditya can you clean the streamlit code before we add this to the readme?

Currently there are a lot of commented code lines, and some general refactor could be done as well, for instance, you have very helpful comments explaining what is the purpose of the function, those should be docstrings.

Keep in mind that this is going to be our front page for deployment, so the code needs to be as clean as possible

@oke-aditya
Copy link
Contributor Author

Yes I will clean it up. Some final touch up is needed.

@oke-aditya
Copy link
Contributor Author

  • Done ✔️ You can have a look here
    Maybe a bit more refactor can be done, But for now I guess this should be good enough.
    @ai-fast-track can you have a look at it ?
    Let me know if something needs more change.

@oke-aditya oke-aditya requested a review from ai-fast-track July 11, 2020 17:25
@lgvaz
Copy link
Collaborator

lgvaz commented Jul 11, 2020

Alright, much better! But there are still some things

  • Repeating the big list OBJECTS_TO_DETECT in both config.py and app.py
  • config.py: # IMG_SIZE = (224, 224) # If you need a image size what does this mean?
  • duplicate code between app.py and model.py
  • duplicate code between app.py and object_detection_app.py
  • duplicate code in utils.py and other files

@lgvaz
Copy link
Collaborator

lgvaz commented Jul 11, 2020

Also, are you doing imagenet normalization? You can use normalize_imagenet

@oke-aditya
Copy link
Contributor Author

Let me clear the code duplication issue.
app.py is a standalone app that should run when user does streamlit run https://raw.githubusercontent.com/oke-aditya/mantisshrimp_streamlit/master/app.py

This file cannot have relative imports etc. So I need to paste all the code in src into here.
The src folder is provided so that user can design his own app easily, modularize it into files and easily hack the script to make a new app.

So ignore the duplication with app and others.
IMAGE_SIZE I will correct it.
ImageNet normalization, I need it but where should I add it let me know. It works without it though.

@ai-fast-track
Copy link
Collaborator

@oke-aditya, I think we need to have just one single app.py. I understand we had the src/...app.py before the prospect of having the remote app.py, and now we can eliminate code duplication.

I just tested the 2 options, they both work: So one code base, let's keep it DRY 😃 .

Option 1: Local
streamlit run app.py

Option 2: Local
streamlit run https://raw.githubusercontent.com/oke-aditya/mantisshrimp_streamlit/master/app.py

Some changes:

I think this should be changed in app.py
APP_NAME = "app.py" instead of "src/object_detection_app.py"

In line 217, mantiss should be replaced by mantis (one s)

In line 258: add ? to st.sidebar.title("What to do")

in line 307: Replace You by Your caption=f"You amazing image has shape {image.shape[0:2]}",

In Info.md file:

Replace To run click on Run app in sidebar. by To run the app, select Run app from the combo box on the sidebar (on the left).

@oke-aditya
Copy link
Contributor Author

ok. I will make the suggestd changes. If I keep only the app.py. Should I keep files such as src/utils.py, src/model.py etc ?

@ai-fast-track
Copy link
Collaborator

ai-fast-track commented Jul 11, 2020

I think It would be better to remove them, and in the app.py highlight the different sections corresponding to each original files (model, utils, etc.) in order to visually distinguish each of those original components.

Something as simple as this (or similar):

################################################################################################
#
# Model Section
#
################################################################################################
#
# Use the mantisshrimp Pytorch model here.
# Simply create the model and get its predictions.

Great Job!

@oke-aditya
Copy link
Contributor Author

Ok. I will remove that too from master branch. Instead have master branch only with app.py from where users can run.
And have this modularized code in the dev branch. This enables me to easily fix up the code when required, and add features if needed.

@oke-aditya
Copy link
Contributor Author

Done. I have updated the master branch. It has only app.py. I tested it. It is working. Also, we on dev branch the modularized code which a user might need.

@ai-fast-track ai-fast-track merged commit 1cdcbb5 into airctic:master Jul 12, 2020
@oke-aditya oke-aditya deleted the add_streamlit_readme branch July 20, 2020 18:58
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.

3 participants