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

Fixes _pack_ being returned as a float #282

Merged
merged 1 commit into from
Mar 1, 2022
Merged

Fixes _pack_ being returned as a float #282

merged 1 commit into from
Mar 1, 2022

Conversation

kdschlosser
Copy link
Contributor

This is going to require a new release to be made ASAP. When the packing is set to a float ctypes raises an error stating that the pack value has to be a non-negative number.

This is going to require a new release to be made ASAP. When the packing is set to a  float ctypes raises an error stating that the pack value has to be a non-negative number.
@@ -130,7 +130,9 @@ def calc_packing(struct, fields):
else:
if pack is None:
return None
return pack/8

return int(pack / 8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be just pack // 8? So there is no any type conversion then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No you cannot. the // syntax is not Python 2.7 compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the // operator is for floor division not truncated. wrapping the value in int() will truncate the value at the decimal position,

Take this as an example. -1 / 2 is -0.5 and when you do -1 // 2 is -1.0 but when you do int(-1 / 2) the result is 0 While I know this case shouldn't apply because the _pack_ attribute is not going to me a negative number at all.

If someone is to look at the code and know the above they will understand that truncation of a negative number is going to "round up" to the closest whole integer and truncation of a positive number is going to "round down" to the nearest whole integer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, I've just tested it on both Python 2.7 and 3.6 and // is for int division. It is backported to Py2.7 a long time ago (I tested it even on Python 2.7.5).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rounding is impossible in this case. The loop iterates over these values: for pack in [None, 16*8, 8*8, 4*8, 2*8, 1*8]: (all are divided by 8).

Copy link
Contributor Author

@kdschlosser kdschlosser Feb 17, 2022

Choose a reason for hiding this comment

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

I think this is an actual bug in Python itself.

Ru the code below.

for pack in [16 * 8, 8 * 8, 4 * 8, 2 * 8, 1 * 8]:
    print(pack, type(pack), pack / 8, type(pack / 8))

if I divide an int by and int why is a float given as a result instead of an int
It doesn't follow the same behavior as // where if the values are int then the returned value is an int.

That's a bug in Python, it is also one that consumes more resources because all numbers are type converted to float or double in c code before the division gets done. If I am dividing an int into an int I expect integer math to be used not floating point math. If one or both of the values are float then floating point math should be used.

I wonder if Python 2 works that way. for some reason I remember integer division returning an int at some point in Python. I remember specifically having to make one of the values a float if I wanted the returned value to be fractional (float)

That is probably what __future__division probably changes It turns Python 3 division into Python 2 division where if 2 integers are used and integer is what gets returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That stackexchange post you linked to is incorrect in so many ways.

// is NOT integer math. I do not know why people think that it is. It is floating point math and the returned value is floored.

https://www.w3schools.com/python/python_operators.asp

In Python when 2 integers use that operator the values are type converted to float or double in c code the division is done and then the result is floored. If the 2 values fed in are integers that the floored result is then type converted to an integer.

integer division is truncated division NOT floored division. // is floored division.

this code

from math import floor


print(-5 / 2)
print(-5 // 2)
print(int(-5 / 2))
print(floor(-5 / 2))

produces these results

-2.5
-3
-2
-3

flooring is rounding down to the closest whole number (over simplified) where as type converting from a fractional number to an integer truncates the number at the decimal position.

-2.5 is -2 when type converted to an integer where as -2.5 when floored and then type converted to an integer is going to be -3.

I am correct about integer division in different Python versions. In Python 2.7 or older if 2 integers are used in division an integer is returned where as in Python 3 a float is always returned. importing division changes Python 2.7's behavior so that dividing 2 integers will return a float. This is backwards from what we want so the correct way is to wrap the division in int to type convert the returned float into an integer which truncates the fractional returned value from the division. This is what Python 2.7's behavior is by default and that is how the original function was intended to work and to duplicate that a type conversion from float to int needs to be done in order to replicate what is done in Python 2.7

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I agree with current fix (sorry for long reply, hope you understand the situation). Just want to ask how about adding from __future__ import division as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I just said. __future__division turns python 2.7 integers into python 3.x's floats.

so if you do 8 / 2 in python 2.7 the returned value is 4 where as in python 3.x it is 4.0, by importing division you change the behavior in python 2.7 so it will return a float like it does in python 3.x. We need to set _pack_ to in integer and not a float.

by wrapping the division with int in python 3.x a type conversion will take place and when run in python 2.7 it won't do anything because the returned value from the division is already an int.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, merged. Should I make a release? Or wait for enabling tests? Maybe it's worth releasing 1.1.12 soon while GitHub hasn't blocked Russia. Anyway I will try to find VPN for continuing contribution to open source.

@vasily-v-ryabov vasily-v-ryabov merged commit b47bea7 into enthought:master Mar 1, 2022
@kdschlosser
Copy link
Contributor Author

If you need I can set up a VPN for you. I have a cisco VPN router and I can set up a tunnel for ya. I can do a manual VPN with an assortment of encryption types. It also has support for OpenVPN and EasyLink VPN

@vasily-v-ryabov
Copy link
Collaborator

Thanks! Currently all is well with GitHub. So I'm planning to make a release on Friday.

@kdschlosser
Copy link
Contributor Author

Let me know if you need it set up. I also have 2 dedicated servers with a 1gb link (up and down) to the internet. One of the servers is not being used and is sitting idle. It has a static IP and I can easily put OpenVPN server on it.

@kdschlosser kdschlosser deleted the fixes_pack branch March 2, 2022 13:33
@vasily-v-ryabov
Copy link
Collaborator

1.1.12 is out. Fortunately GitHub decided not to block us. Anyway thanks for willing to help. Really appreciate it!

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