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

[python3] Use floor division where necessary. #247

Merged
merged 1 commit into from
Mar 7, 2017

Conversation

hgaiser
Copy link
Contributor

@hgaiser hgaiser commented Jan 24, 2017

As per the docs, some divisions need to be floored (as opencv expects integers in these places). This change is python 2 / 3 compatible.

@vrabaud
Copy link
Contributor

vrabaud commented Jan 29, 2017

Thx for //
For BytesIO, would you be able to read something that was written with StringIO ?

@hgaiser
Copy link
Contributor Author

hgaiser commented Jan 31, 2017

Not sure to be honest. The main issue was that the StringIO was used to write an encoded image (as a sequence of bytes) and the yaml and text files (as a string). In python2, a bytes array or string array are the same thing, which is why StringIO was applicable. In python3 however they are two separate things (try to evaluate bytes is str in python2/3). Therefore, StringIO was giving an error when writing the image, since it was expecting a string but received a bytesarray. The solution I chose here was to write the yaml files as bytes too and use a BytesIO instead.

Apologies for not explaining this change earlier.

@vrabaud
Copy link
Contributor

vrabaud commented Jan 31, 2017

Can you just remove that commit for now and make another one with StringIO please ? I want to merge the first commit for now and try the other one.

@hgaiser
Copy link
Contributor Author

hgaiser commented Feb 3, 2017

Done.

@hgaiser
Copy link
Contributor Author

hgaiser commented Mar 7, 2017

Any update on this ?

@vrabaud vrabaud merged commit 870a897 into ros-perception:indigo Mar 7, 2017
@vrabaud
Copy link
Contributor

vrabaud commented Mar 7, 2017

Thx for cleaning it !

@vrabaud
Copy link
Contributor

vrabaud commented Mar 7, 2017

time for doing one with StringIO :)

@hgaiser
Copy link
Contributor Author

hgaiser commented Mar 7, 2017

No problem :) thanks for merging.

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.

None yet

2 participants