Skip to content

ENH: hard-code finfo parameters for known types #8504

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

Merged
merged 8 commits into from
Feb 3, 2017

Conversation

matthew-brett
Copy link
Contributor

Hard-code the MachAr parameters for float64, float32, 80-bit extended
precision, to save time, and provide skeleton for more difficult types
such as the double double on PPC; see
#2669

@matthew-brett
Copy link
Contributor Author

There's a test error from test_warnings.py in https://travis-ci.org/numpy/numpy/jobs/193586749#L3330 . It's telling me I'm not allowed to use warnings.simplefilter('ignore') - which I used here. It seems like a perfectly sensible use to me - why is test_warnings complaining?

@charris
Copy link
Member

charris commented Jan 20, 2017

Hmm, not sure. @seberg Thoughts?

EDIT: I suspect that there is no guarantee that the warning filter will be properly reset, but exactly why that would be the case here I'm not sure. You could probably use with suppress_warnings() instead of with catch_warnings():, but as both are currently used I'm not clear on when the new version is required.

@charris
Copy link
Member

charris commented Jan 20, 2017

    Context manager and decorator doing much the same as
    ``warnings.catch_warnings``.

    However, it also provides a filter mechanism to work around
    http://bugs.python.org/issue4180.

    This bug causes Python before 3.4 to not reliably show warnings again
    after they have been ignored once (even within catch_warnings). It
    means that no "ignore" filter can be used easily, since following
    tests might need to see the warning. Additionally it allows easier
    specificity for testing warnings and can be nested.

So the "ignore" is the problem. Note also that the warning filters are not thread safe, so should be avoided outside of tests when possible.

@matthew-brett
Copy link
Contributor Author

Sure - but how should I do what I'm trying to do if I'm not allowed to ignore the warning inside the context manager block?

@matthew-brett
Copy link
Contributor Author

OK - found a suppress_warnings incantation that seems to work.

@njsmith
Copy link
Member

njsmith commented Jan 20, 2017

What's the warning here?

@seberg
Copy link
Member

seberg commented Jan 20, 2017

Well, to be honest, for some code doing funny warning stuff may make sense and the test should likely be relaxed to only tests. On the other hand, I think np.seterr is probably the correct thing here, since it seems to me that this should be floating point stuff (and I think seterr is even threadsafe? frankly not sure).

@seberg
Copy link
Member

seberg commented Jan 20, 2017

Or if the warning makes sense/does not matter, moving the context to the tests makes more sense.

@pv
Copy link
Member

pv commented Jan 20, 2017 via email

@njsmith
Copy link
Member

njsmith commented Jan 20, 2017

seterr is threadsafe yeah:

thedict = PyThreadState_GetDict();

@mraspaud
Copy link

@matthew-brett on ppc64, I get a mantissa (significand) size of 116 bits for long doubles (double double in this case). But you seem to set it to 105, (105, 1024, 16), may I ask why ?

@matthew-brett
Copy link
Contributor Author

OK - I used the errstate context manager instead, that does look better.

@matthew-brett
Copy link
Contributor Author

@mraspaud - I got the number of digits from looking at the eps value found by using np.nextafter(np.longdouble(1), np.longdouble(2)). Actually, the number also makes some sense, if we suppose that the number of digits comes from the sum of the number of significand digits in both doubles = 53 + 53 - 1 for the implicit first.

@charris
Copy link
Member

charris commented Jan 20, 2017

I was thinking of something more along the lines of the build time determination, basically viewing a value as a string and then looking that up. See numpy/core/setup_common.py.

@matthew-brett
Copy link
Contributor Author

Chuck -

a) I think we'd need most of this machinery anyway, to replicate the old finfo behavior. Then the only difference between build-time and run-time detection is the small fragment of code here to detect the eps and huge and byte size values, and select accordingly;
b) I can't explain the mechanism, but I believe I have seen instances where the type of longdouble changed at run time - see comment here.

@matthew-brett
Copy link
Contributor Author

Other somewhat trivial advantage of this approach - finfo is a lot faster. With this PR:

In [2]: time np.finfo(np.float64)
CPU times: user 189 µs, sys: 23 µs, total: 212 µs
Wall time: 218 µs

With 1.12 release wheel:

In [2]: time np.finfo(np.float64)
CPU times: user 12.1 ms, sys: 308 µs, total: 12.4 ms
Wall time: 12.3 ms

Of course, this is a one-time only cost, as the results are cached.

@matthew-brett
Copy link
Contributor Author

@mraspaud - also see https://en.wikipedia.org/wiki/Quadruple-precision_floating-point_format#Double-double_arithmetic - which says these have 106 digits in the significand. The numpy figures for significand digits are all N-1, I guess omitting the implicit first. Also see : http://mrob.com/pub/math/f161.html (the paper explains that using the sign bit of the second double makes the actual number 107...).

@matthew-brett
Copy link
Contributor Author

Longdoubles are especially slow in the old implementation:

In [3]: time np.finfo(np.longdouble)
CPU times: user 150 ms, sys: 2.02 ms, total: 152 ms
Wall time: 161 ms
Out[3]: finfo(resolution=1e-18, min=-1.18973149536e+4932, max=1.18973149536e+4932, dtype=float128)

Same command takes 263 microseconds from code in this PR.

@mraspaud
Copy link

@matthew-brett indeed, I was just mislead by HDF5 reporting 116 bits significand...

@matthew-brett
Copy link
Contributor Author

Chuck - would you consider merging this one, with a hoped-for follow-up to do build-time detection?

@charris
Copy link
Member

charris commented Jan 24, 2017

@matthew-brett I'll take a look at this later today. I wasn't actually suggesting doing build-time detection, but rather doing the same sort of type detection at runtime. It is/can be all done in python. But I will need to take a closer look here before deciding if it is worthwhile.

@matthew-brett
Copy link
Contributor Author

OK - thanks. If you're unhappy with the float32, float64, float80 detection, I'm happy to drop those, leaving only the PPC longdouble, which does need a fix.

Check that finfo returns somewhat reasonable parameters for all floating
point types.
@matthew-brett
Copy link
Contributor Author

@mraspaud - can you test the results of:

nosetests path/to/numpy/core/tests/test_getlimits.py

with the current branch?

@matthew-brett
Copy link
Contributor Author

I think this one is ready now. The detection doesn't cover IEEE float128, hence the lack of warning when the byte-string detection fails and falls back to the discovery code. If anyone has an AIX machine they have access to, it would be good to check on that - but at least, this code falls back to the previous code when it fails to detect a signature, so it shouldn't be worse at detection.

@matthew-brett
Copy link
Contributor Author

Planning on getting access to a SPARC with actual IEEE 754 128-bit floats to test against.

@mraspaud
Copy link

mraspaud commented Feb 2, 2017

(h5py)debian@debian8-ppc64el:~$ nosetests numpy/numpy/core/tests/test_getlimits.py 
.............
----------------------------------------------------------------------
Ran 13 tests in 0.062s

OK

if key in _KNOWN_TYPES:
return _KNOWN_TYPES[key]
# Fall back to parameter discovery
return _discovered_machar(ftype)
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth raising a warning here so that we might get reports of new types we should cover.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe include the missing key in the message.

Warn if we don't have a signature for the float type.
Still needs test on platform with IEEE float128, to check MachAr-like
parameters are correct.
@pv
Copy link
Member

pv commented Feb 3, 2017 via email

@charris
Copy link
Member

charris commented Feb 3, 2017

@pv The C code just writes a structure that is then read and analysed in python, but is otherwise similar to the current code. Nextafter isn't really defined for double_double, so depends on the library choices, although the libraries are likely to follow the IBM recommendation. If we ever expose the build time macros the code here can be modified, but as it stands should serve as good check for hardcoding the types. The only thing that worries me a bit is that the -0.1 value is not exactly representable, so the result may depend on the library implementation. OTOH, it is a test of the libraries.

@charris
Copy link
Member

charris commented Feb 3, 2017

I should say that by C code analysed is the object file and the Python code looks for the structure in that file.

@matthew-brett
Copy link
Contributor Author

Chuck - I believe (though happy to be corrected) that all the floating point implementations specify a unique correspondence between bit pattern and input number (here -0.1). I mean, they guarantee the closest representable number, and there is only one bit pattern for any given representable number. I can see how there might be more than one pattern for double double (1.0 + 0.01 == 1.1 - 0.09), but I haven't looked into it. The patterns we're getting also match with those recorded in the perl configure script I pointed to - here - so I guess we can depend on them, but in any case, all that will happen, if they are wrong, is that the code will return to the original behavior.

@matthew-brett
Copy link
Contributor Author

Tests pass on SPARC, with IEEE float128. Ready to merge from my point of view.

@charris charris merged commit a611932 into numpy:master Feb 3, 2017
@charris
Copy link
Member

charris commented Feb 3, 2017

In it goes, thanks @matthew-brett .

@charris
Copy link
Member

charris commented Feb 3, 2017

How do the timings compare?

@matthew-brett
Copy link
Contributor Author

Timings, macOS Intel, numpy 1.12:

In [3]: time np.finfo(np.longdouble)
CPU times: user 139 ms, sys: 2.43 ms, total: 141 ms
Wall time: 149 ms

This branch:

In [2]: time np.finfo(np.longdouble)
CPU times: user 165 µs, sys: 23 µs, total: 188 µs
Wall time: 195 µs

@ahaldane
Copy link
Member

@matthew-brett This commit is causing me a bit of trouble as described in this comment.

The problem revolves around the fact that you are calling numpy functions before numpy modules are fully loaded here, specifically you are calling array2string. This causes cyclic import issues.

Do you think we might delay the evaluations here until after numpy modules are fully loaded?

@matthew-brett
Copy link
Contributor Author

@ahaldane - does this work? #9113

@tacaswell
Copy link
Contributor

Just to verify, this went is for 1.13 and did not get backported further?

@mattip
Copy link
Member

mattip commented Oct 24, 2018

@tacaswell git tag --contains a611932bbcb132 shows 1.13.0 is the earliest tag with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants