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

update reference files for new asdf standard versions #409

Merged
merged 4 commits into from
May 13, 2024

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Nov 10, 2023

Fixes a bug in the reference file generator where generated files used the latest asdf standard.

Generates and adds reference files for asdf standard versions 1.1.0 through 1.5.0 (inclusive).

I've added a few comments describing changes to the reference files (and the script to generate them).

!core/complex-1.0.0 (nan+infj), !core/complex-1.0.0 (nan-infj), !core/complex-1.0.0 -1.79769313486e+308j,
!core/complex-1.0.0 1.79769313486e+308j, !core/complex-1.0.0 2.22044604925e-16j,
!core/complex-1.0.0 1.11022302463e-16j, !core/complex-1.0.0 2.22507385851e-308j,
!core/complex-1.0.0 (nan+infj), !core/complex-1.0.0 (nan-infj), !core/complex-1.0.0 -1.7976931348623157e+308j,
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 values in the existing complex.yaml are not correct for a 64 bit float (real or imaginary portion of a complex 128, yaml will treat all numbers as 64 bit so the expanded representation should be seen for all bit-depths of complex values in this file).

>> np.finfo(np.float64).max == 1.79769313486e+308
False
>> np.finfo(np.float64).max == 1.7976931348623157e+308
True

datatype<U: !core/ndarray-1.0.0
data: ['', "\U00010020"]
datatype: [ucs4, 2]
datatype: [ucs4, 1]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unclear to me what unicode_spp is supposed to be illustrating. My closest guess is that the character 𐀠 is U+10020 is part of plane 1 the Supplementary Multilingual Plane so perhaps this was suppored to be smp not spp. On my system this is stored in 4 byte unicode which explains the [ucs4, 1] change in this file. I'm not sure what system or version of numpy was using 8 bytes to store this character.

>> np.unicode_('𐀠').dtype.itemsize
4

The generate.py file could be updated to use U2 instead of U to generate a file that uses 8 bytes:

>> np.array(['𐀠'], dtype='U2').dtype.itemsize
8

At this point I suggest we hold off making this change to generate.py and instead include the updated file in this PR as it offers a slightly different example for unicode in ASDF compared to the bmp file (which has a combined character which results in [ucs4, 2]).

@@ -155,8 +155,8 @@ def generate(version):
filename = os.path.join(outdir, name)
func(filename + ".asdf")
with asdf.open(filename + ".asdf") as af:
af.resolve_and_inline()
af.write_to(filename + ".yaml")
af.resolve_references()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces the only use of resolve_and_inline I'm able to find in any package outside of asdf. See asdf-format/asdf#1690 for a description of the resolve_and_inline deprecation which includes the suggestion to update code similar to the update in this PR (replace resolve_and_inline with resolve_references and all_array_storage='inline').

@braingram braingram marked this pull request as ready for review November 20, 2023 21:41
@braingram braingram requested a review from eslavich December 13, 2023 16:22
Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram braingram merged commit c367d14 into asdf-format:main May 13, 2024
14 checks passed
@braingram braingram deleted the update_ref_files branch May 13, 2024 18:49
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