Skip to content

Remove useless global variable in custom_utils #454

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.

Sign up for GitHub

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

Open
belotfa wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Remove useless global variable in custom_utils #454

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6681a02
Remove useless global variable in custom_utils
belotfa Jan 15, 2020
c14b135
Modify arguments
belotfa Jan 15, 2020
42f8f1f
Update requirements.txt
belotfa Jan 16, 2020
3aeddb6
Merge pull request #2 from belotfa/belotfa-patch-1
belotfa Jan 16, 2020
73a703b
Update requirements to downgrade Pillow version.
belotfa Jan 22, 2020
54c7ace
Update custom_utils.py
belotfa Feb 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions imageai/Prediction/Custom/custom_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import json

CLASS_INDEX = None



def preprocess_input(x):
"""Preprocesses a tensor encoding a batch of images.
Expand All @@ -24,11 +21,7 @@ def preprocess_input(x):

def decode_predictions(preds, top=5, model_json=""):


global CLASS_INDEX

if CLASS_INDEX is None:
CLASS_INDEX = json.load(open(model_json))
CLASS_INDEX = json.load(open(model_json))
Copy link
Copy Markdown
Contributor

@rola93 rola93 Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I agree current implementation is not the best. However, this alternative isn't that better: each invocation on it will load exactly the same json.

If will let this change, should at least add a with block to make sure the file is closed properly:

with open(model_json) as f:
    CLASS_INDEX = json.load(f)

Agree on removing as much global variables as possible.

I don't fully understand why was on a global variable while the filename is a parameter... doesn't make sense for me, maybe I'm not seeing something.

Copy link
Copy Markdown
Author

@belotfa belotfa Feb 8, 2020

Choose a reason for hiding this comment

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

I agree current implementation is not the best. However, this alternative isn't that better: each invocation on it will load exactly the same json.

Yeah but that's my point actually. With several instances of CustomImagePrediction running, each invocation will not load exactly the same json and the global variable CLASS_INDEX prevents us from running these instances at the same time.

I don't fully understand why was on a global variable while the filename is a parameter... doesn't make sense for me, maybe I'm not seeing something.

This implementation is useful in other parts of codes in other "custom_utils.py" where some functions don't give a model_json in their parameters (but I checked, all functions that call this "custom_utils.py" give it).

PS : I may have misunderstood your concern, english is not my first language...

Copy link
Copy Markdown
Contributor

@rola93 rola93 Feb 10, 2020

Choose a reason for hiding this comment

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

My point is tat previous version loaded the file only once. We should go to an implementation with only one file-reading but avoiding the point you mention. A bigger refactor is needed, as I see.

English is not my first language too

thanks for your PR, we need to wait for @OlafenwaMoses now

results = []
for pred in preds:
top_indices = pred.argsort()[-top:][::-1]
Expand All @@ -38,4 +31,4 @@ def decode_predictions(preds, top=5, model_json=""):
each_result.append(pred[i])
results.append(each_result)

return results
return results
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
tensorflow
keras
numpy
pillow
pillow<7.0.0
scipy
h5py
matplotlib
Expand Down