Skip to content

Conversation

@astrofrog
Copy link

@astrofrog astrofrog commented Nov 29, 2022

Fixes #14
Fixes #40

@github-actions
Copy link

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@astrofrog astrofrog changed the title Add quantization codec Add quantization codec and implement compression/decompression of floating point data Dec 1, 2022
@astrofrog astrofrog mentioned this pull request Dec 1, 2022
@astrofrog
Copy link
Author

I think lossy tests are close to passing but due to the issue mentioned in #40 the floating point values don't match exactly:

E       Mismatched elements: 144 / 144 (100%)
E       Max absolute difference: 7.677353
E       Max relative difference: 7.3442245
E        x: array([[163.9622  ,  12.696566,   2.805807,  83.07929 ,   3.645739,
E                12.367053, 118.553955,  33.24904 ,  59.632137, 209.43936 ,
E                23.05459 ,  67.77326 ],...
E        y: array([[1.626797e+02, 9.415493e+00, 1.536737e+00, 8.256937e+01,
E               5.992008e+00, 9.394189e+00, 1.165274e+02, 3.576727e+01,
E               6.091895e+01, 2.120618e+02, 2.193155e+01, 6.678799e+01],...

I think we should adjust the tests to compare the data read in by both libraries rather than comparing to the very original data.

@astrofrog astrofrog force-pushed the quantization branch 2 times, most recently from 42d9834 to d55034b Compare December 2, 2022 14:28
@astrofrog astrofrog marked this pull request as ready for review December 2, 2022 14:47
@astrofrog astrofrog requested a review from Cadair December 2, 2022 14:47
"""


class Quantize(Codec):
Copy link

Choose a reason for hiding this comment

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

I don't think we can have this inherit Codec if we aren't going to implement encode and decode as it will fail the ABC check if numcodecs is importable.

Copy link
Author

Choose a reason for hiding this comment

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

That's fine and we are going to remove references to Codec anyway so we can just change this to not inherit from anything?

)
settings["itemsize"] = tile_data.size // int(np.product(actual_tile_shape))

lossless = len(cdata) == 0
Copy link

Choose a reason for hiding this comment

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

This might want a better variable name, you can have lossless data in the COMPRESSED_DATA column if it was compressed directly with GZIP without quantization? Am I right in thinking that this is "this tile is losslessly compressed when the others are quantized"?

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's correct

Copy link
Author

Choose a reason for hiding this comment

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

gzip_fallback?

astrofrog and others added 2 commits December 3, 2022 10:48
Co-authored-by: Stuart Mumford <stuart@cadair.com>
@astrofrog
Copy link
Author

@Cadair - I found some time to implement the comments

@astrofrog astrofrog merged commit 65afc9c into aperiosoftware:fits-byte-compress-decompress Dec 4, 2022
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.

2 participants