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

auto inline for large integers #891

Closed
CagtayFabry opened this issue Dec 1, 2020 · 8 comments · Fixed by #894
Closed

auto inline for large integers #891

CagtayFabry opened this issue Dec 1, 2020 · 8 comments · Fixed by #894

Comments

@CagtayFabry
Copy link
Contributor

CagtayFabry commented Dec 1, 2020

During testing against the current asdf master we noticed some test failures due to the change introduced in #882 .

Since previous asdf versions defaulted to store arrays in blocks, it was "safe" to write arrays with large (>2^51) integer values using default settings.
On the current version, the default settings might fail to write the asdf file because the default value for auto_inline tries to inline smaller arrays that may contain large integer values (e.g. ns timestamps since 1970)

Here is a simple example that works on 2.7.1 but fails with current auto_inline

import asdf
import numpy as np

with asdf.AsdfFile(tree={"test": np.array([1, 2**51])}) as af:
    af.write_to("test.asdf")

I think it would be good for asdf to allow this special case with default parameters.
One solution might be to specifically check integer arrays that should get inlined because of the current auto_inline setting (should be "cheap" since they are short) and store them as binary block if large values are encountered. Maybe issue a warning in this case.
I have tested a quick implementation here: master...CagtayFabry:fix_large_integer_inline

If the user specifically requests "inline" for a specific block or the whole file, I think it is reasonable to throw the error.

What do you think @eslavich @kmacdonald-stsci ?

@eslavich
Copy link
Contributor

eslavich commented Dec 1, 2020

Thanks for the report and the solution as well! I'll bring this up at our meeting this morning. I think your proposed fix is reasonable.

@eslavich
Copy link
Contributor

eslavich commented Dec 1, 2020

We discussed this and @perrygreenfield pointed out that the 52-bit limit on integers in the YAML may not really be necessary. According to the ASDF Standard the limit is there for the sake of javascript, but we don't have a javascript ASDF reader or near-term plans to implement one. It seems reasonable to demand that ASDF readers handle 64-bit integers and have the javascript implementation manage its own quirks, rather than restrict the standard.

@CagtayFabry
Copy link
Contributor Author

This sounds like the ideal solution.
Would this be hard to implement? This is probably not for the 2.8 release I assume? Do you think we could implement the above mentioned fix for the 2.8 release until the 52-bit limit will be removed?

@eslavich
Copy link
Contributor

eslavich commented Dec 2, 2020

I think we can probably remove the 52-bit limit in the 2.8 release, we're already introducing a new version of the standard so it would just be a matter of deleting some language from the standard documentation and removing a small amount of code from the Python library.

@perrygreenfield
Copy link
Contributor

I was hoping Ed would say that.

@CagtayFabry
Copy link
Contributor Author

sounds great, thank you 👍

@kmacdonald-stsci
Copy link
Contributor

I have made edits that remove the 52 bit limit, but we will still have a 64 bit limit. To see exactly what the limits are, you can look at asdf/constants.py in the pull request at the link below.

#894

@CagtayFabry
Copy link
Contributor Author

thanks @kmacdonald-stsci , having the current min/max supported numbers in asdf/constants.py will certainly help in the future

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 a pull request may close this issue.

4 participants