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

Literal bit limit changed from 52 bits to 64 bits #894

Merged
merged 13 commits into from
Jan 15, 2021
Merged

Literal bit limit changed from 52 bits to 64 bits #894

merged 13 commits into from
Jan 15, 2021

Conversation

kmacdonald-stsci
Copy link
Contributor

@kmacdonald-stsci kmacdonald-stsci commented Dec 7, 2020

Removal of the 52 bit limit for numbers in an ndarray (originally imposed for anticipated use with javascript), which allows usage for larger numbers. The ndarray is still limited by 64 bits by the dtype. In addition to removing this validation check, the pytest tests used to test the original limit have been removed as well.

Resolves #891

Copy link
Contributor

@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Looks good. Just a minor comment and question below. 🚀

asdf/tests/test_api.py Outdated Show resolved Hide resolved
asdf/tests/test_schema.py Outdated Show resolved Hide resolved
@eslavich
Copy link
Contributor

What should we do about uint64 arrays that are small enough to be auto inlined? For example this:

import numpy as np
import asdf

arr = np.array([2**64 - 1], dtype=np.uint64)

with asdf.AsdfFile({"arr": arr}) as af:
  af.write_to("test.asdf")

results in the following error:

ValidationError: Integer value 18446744073709551615 is too large to safely represent as a literal in ASDF

@@ -401,24 +401,6 @@ def test_auto_inline_masked_array(tmpdir):
assert len(list(af.blocks.internal_blocks)) == 2


def test_auto_inline_large_value(tmpdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this test and rework it to cover the np.uint64 case?

@jdavies-st jdavies-st added this to the 2.8.0 milestone Jan 14, 2021
@kmacdonald-stsci kmacdonald-stsci merged commit b6e1d69 into asdf-format:master Jan 15, 2021
@kmacdonald-stsci kmacdonald-stsci deleted the inline_literal_bit_limit branch January 15, 2021 18:02
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.

auto inline for large integers
3 participants