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

python rounding errors fix #2306

Closed

Conversation

its-pointless
Copy link
Contributor

fixes #2236

Copy link
Contributor

@tomty89 tomty89 left a comment

Choose a reason for hiding this comment

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

Is there any reason for you not to patch pyport.h directly (add #undef PY_NO_SHORT_FLOAT_REPR after the two lines where it could be defined)?

From your PR I assume only the first one is of our concern:
https://github.com/python/cpython/blob/3.6/Include/pyport.h#L476
For that there are three possible cases that could avoid the case, what makes you think the little endian one is appropriate for all possible platforms (especially for arm and arm64)?

There's a reason that none of the three is defined for cross-compiling, because tests are performed in configure when built on site, which can't be done for the target platform when cross-compile.

You should patch configure.ac instead even if this is necessary and appropriate:
https://github.com/python/cpython/blob/3.6/configure.ac#L4272

@tomty89
Copy link
Contributor

tomty89 commented Apr 3, 2018

Or configure if we don't regenerate it:
https://github.com/python/cpython/blob/3.6/configure#L13978

@tomty89
Copy link
Contributor

tomty89 commented Apr 3, 2018

Also python2?

@its-pointless
Copy link
Contributor Author

1 python2 doesn't even attempt it. seriously 1/10 is 0. Even on normal linux pc. luckly this has nothing to do with the actual calculations going on.
2. i was under the impression that DOUBLE_IS_LITTLE_ENDIAN_IEEE754 on android. ...

@tomty89
Copy link
Contributor

tomty89 commented Apr 3, 2018

https://github.com/python/cpython/blob/2.7/Include/pyport.h#L611

Also:

$ python2
Python 2.7.14 (default, Jan  7 2018, 04:00:23)
[GCC 4.2.1 Compatible Android Clang 5.0.300080 ] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> 1/10
0
>>> 1.0/10.0
0.10000000000000001
>>> 1/10.0
0.10000000000000001
>>> 1.0/10
0.10000000000000001
>>>

You need to make at least one of the numbers a float if you want to perform a float calculation in python2. For the record:

$ python
Python 3.6.4 (default, Jan  7 2018, 03:52:16)
[GCC 4.2.1 Compatible Android Clang 5.0.300080 ] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> 8/4
2.0
>>>
$ python2
Python 2.7.14 (default, Jan  7 2018, 04:00:23)
[GCC 4.2.1 Compatible Android Clang 5.0.300080 ] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> 8/4
2
>>>

It has nothing to do with the OS but CPU I think.

DOUBLE_IS_LITTLE_ENDIAN_IEEE754 should be fine on most cases, it's just I wonder if there could be any corner ones that Termux might be serving, and whether the is a necessity to define it instead of enforcing short repr directly.

@its-pointless
Copy link
Contributor Author

Would it differ vs gcc?
it would have to do with the os if the vendor of the os stipulates behavior for vendors to adhere to. We use the ndk as its a standard. I wonder how many other packages assume a standard from cpus?

@tomty89
Copy link
Contributor

tomty89 commented Apr 3, 2018

I don't think the compiler has anything to do with it.

And I don't know. The thing is the question stands anyway: we don't we just undefine PY_NO_SHORT_FLOAT_REPR directly instead? It will be less confusing as well. Does it not work?

And if that's the case, we will go for defining DOUBLE_IS_LITTLE_ENDIAN_IEEE754 by patching configure. Simple, isn't it?

@fornwall
Copy link
Member

fornwall commented Apr 3, 2018

So, it seems that ac_cv_little_endian_double=yes is enough? This is applied in 8dc5d78 which is applied in the latest package versions (3.6.5-1 for python, and 2.7.14-3 for python2). Seems to work fine for all arches, right?

@its-pointless
Copy link
Contributor Author

it should since java uses ieee754 and from jni.h
/* Primitive types that match up with Java equivalents. /
typedef uint8_t jboolean; /
unsigned 8 bits /
typedef int8_t jbyte; /
signed 8 bits /
typedef uint16_t jchar; /
unsigned 16 bits /
typedef int16_t jshort; /
signed 16 bits /
typedef int32_t jint; /
signed 32 bits /
typedef int64_t jlong; /
signed 64 bits /
typedef float jfloat; /
32-bit IEEE 754 /
typedef double jdouble; /
64-bit IEEE 754 */

@tomty89
Copy link
Contributor

tomty89 commented Apr 4, 2018

IEEE 754 does not specify endianness, that's why there are three DOUBLE_IS_..._ENDIAN_IEEE754.

I think the question is more like, whether assuming support of any of the variants has effect other than preventing PY_NO_SHORT_FLOAT_REPR to be defined.

That's why if we want to avoid unexpected side effect, patching pyport.h should be our first approach, and only if that doesn't work, we assume little endian double, which should work fine on most platforms.

But whatever the patch is up and it's probably the easiest way anyway, so who cares, right?

@its-pointless
Copy link
Contributor Author

ieee754 doesn;t specify endianess but android does. its always little endian.

@tomty89
Copy link
Contributor

tomty89 commented Apr 4, 2018

If you say so.

@SDRausty

This comment was marked as spam.

@its-pointless
Copy link
Contributor Author

No i like tomty89 being around even if disagreements occur as long as they are about the what is best technically its all good. Disagreement is fine if the end point is better. So far that has been the case.

@SDRausty

This comment was marked as spam.

@tomty89
Copy link
Contributor

tomty89 commented Apr 17, 2018

Should I feel lucky that I didn't also delete my reply to your quoted reply:
SDRausty/TermuxArch#54 (comment)
Coz apparently others deleting their comments could drive you mad. How does it matter that I deleted a few of my broken ideas?

I don't have broad knowledge but yes I am picky even in that case, and I do think that that pickiness can be a form of contribution.

And since when I do personal attacks? (Does attacking someone's grammar count? Either way that's not what I did) I mean, can I even do that? Like I pretty much know nothing about any of you guys other than your code.

Can anyone explain what is technically correct about my reply to a non-existent comment?

Yes I can. Because GitHub provides a delete button to whoever made specific comments, so yes, it's technically (and logically) correct that they could vanish at any point. Now would you just stop harassing others just because you think I have an "attitude"? File an issue to my parents or primary school teachers, if you can.

@SDRausty

This comment was marked as spam.

@Grimler91
Copy link
Member

@SDRausty enough already. This pull request is about rounding errors in python and I'm not really not sure what you are trying to achieve here.

I'll delete additional aggressive posts (from any source).

@SDRausty

This comment was marked as spam.

@SDRausty

This comment was marked as spam.

@ghost
Copy link

ghost commented Apr 17, 2018

I am not happy with these actions either

@SDRausty No one is happy there when you posting aggressive offtopic comments, especially if they are posted in pull request closed 2 weeks ago.

what do you do when your projects are attacked?

Github has a special button that allows to block user. More info about this: Github Help.

@SDRausty

This comment was marked as spam.

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.

Rounding errors in python
5 participants