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

Fix for json_response["image"] in prediction.py #93

Merged
merged 10 commits into from Jan 13, 2023

Conversation

Tyler-Odenthal
Copy link
Contributor

Description

There was an issue because we changed our data structure recently. The resulted in json_response["image"] not existing.

Used PIL to get image dimensions and reconstructed img_dims.

No dependency changes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this change been tested, please provide a testcase or example of how you tested the change?

Tested on two TRT scripts and with three different models. Have yet to test it on an edge device, but this fix should not be device specific.

Any specific deployment considerations

Reading image for image dimensions takes time, if there is a more efficient way to get img_dims or if we are already pulling dims somewhere else in the script. We should pass the variable instead of creating another variable.

Tyler Odenthal and others added 5 commits January 5, 2023 16:39
There was an issue because we changed our data structure recently. The resulted in json_response["image"] not existing.

Used cv2.imread to get image dimensions and reconstructed img_dims.
@Tyler-Odenthal Tyler-Odenthal self-assigned this Jan 6, 2023
@Tyler-Odenthal Tyler-Odenthal added the bug Something isn't working label Jan 6, 2023
Tyler Odenthal added 3 commits January 5, 2023 17:23
Sorry for all the updates, forgot to do "make style"
Was able to pass variables from object_detection.py instead of generating in prediction.py
Ran python -m unittest a lot.
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @Tyler-Odenthal

For the future, don't change the version number - only change it in a self contained PR

@@ -8,7 +8,7 @@
from roboflow.core.project import Project
from roboflow.core.workspace import Workspace

__version__ = "0.2.23"
__version__ = "0.2.24"
Copy link
Contributor

Choose a reason for hiding this comment

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

why the version was changed? Are you planning on a new release? It's better to make a single separate PR with the version bump to avoid overlapping with other PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! Appreciate the feedback!

Is this still good to push or do I need to revert the version number?

Choose a reason for hiding this comment

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

Now you have to push it since we increase the number from another PR :)

@Tyler-Odenthal Tyler-Odenthal merged commit 96ce2a0 into main Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants