jzarr read/write (close #36) by joshmoore · Pull Request #37 · zarr-developers/zarr_implementations

joshmoore · 2021-04-26T13:23:01Z

The only real issue (repeated from #25) is that I don't see a way to return an array from the read_from_jzarr method which means that non-Python implementations may need to handle the verification code themselves internally.

cc: @constantinpape

grlee77 · 2021-04-27T18:52:27Z

+            String dsname = args[2];
+            ZarrArray verification = ZarrGroup.open(fpath).openArray(dsname);
+            int[] shape = verification.getShape();
+            if (!Arrays.equals(new int[]{}, shape)) {


Maybe I am reading this wrong, but why does it look like this line is comparing a new empty array to shape? I was expecting Array.equals(SHAPE, shape)? (I am not a Java dev)

Am I correct that when called with arguments, this program only checks the shape, but not the actual image contents?

What do you think about writing out a NumPy .npy or .npz file from Java? (A quick search indicates there is at least one library available for this (GitHub). That is what the xtensor-zarr executable does (using an .npz writer that was already available in xtensor-io). The array comparison to the reference can then done in Python. Or maybe it is just easier to do the verification in java itself instead?

Maybe I am reading this wrong, but why does it look like this line is comparing a new empty array to shape? I was expecting Array.equals(SHAPE, shape)? (I am not a Java dev)

Argh. No, A) you're completely right. I added this before the troubles with <u1 began in order to force a failure so B) why isn't it forcing a failure?!

What do you think about writing out a NumPy .npy or .npz file from Java?

I'm personally skeptical. I think we're going to want to increase the amount of verification done in each language and it seems like that will hit limits via NumPy. But I assume I'm going to lose this one to you and @constantinpape 🙂

see: 1ad9af0

I'm personally skeptical. I think we're going to want to increase the amount of verification done in each language and it seems like that will hit limits via NumPy. But I assume I'm going to lose this one to you and @constantinpape slightly_smiling_face

I am pretty agnostic on this one. I think it's fine if you validate in java. But we should somehow return the correct error codes to the python test to populate the test summary. (I haven't checked the rest of this PR yet, so maybe you are doing that already ;)).

why isn't it forcing a failure?!

I opened joshmoore#1 with a simplified way of calling the subprocess that I am sure is actually running it (it takes a few seconds per test case). That one will fail if the exit code is not zero.

I also do not have a problem with just doing the validation in Java

use subprocess.check_output to call jzarr test script

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

joshmoore · 2021-05-04T19:39:35Z

Looking at one of these failures (reading js.zr/blosc/lz4/ with Zarr) the first few values differ:

Modifying |u1 back to jzarr's <u1 doesn't correct the issue.

cc: @SabineEmbacher

grlee77 · 2021-05-04T20:17:55Z

Looking at one of these failures (reading js.zr/blosc/lz4/ with Zarr) the first few values differ:

Yeah, the values seen there are what would be obtained when trying to convert uint8 values to int8 so that values >=128 overflow and become negative.

For example

import numpy as np
np.asarray([154, 147, 151, 109], dtype=np.int8)

gives

array([-102, -109, -105,  109], dtype=int8)

SabineEmbacher · 2021-05-05T06:59:00Z

JZarr is bound to the primitive data types of Java. There is no primitive unsigned byte data type in Java.
Data types which are offered in the JZarr API (see com.bc.zarr.DataType) only serve to be written into the .zarray file so that the reader of the data knows how to interpret it. When the written data is read with Python, the interpretation happens automatically.
If the data is read again with JZarr, the correct interpretation of the raw data is up to the user.

Here is an example of interpreting byte data as unsigned bytes. org.esa.snap.core.datamodel.ProductData.UByte

The same overflow effect will be observed with the u2 and u4 data types.

joshmoore · 2021-05-05T07:40:38Z

Thanks for the explanation, @SabineEmbacher. I clearly misunderstood https://jzarr.readthedocs.io/en/latest/datatype.html?highlight=unsigned#data-types

joshmoore · 2021-05-05T07:55:47Z

Failures now look to all be related to nesting:

9 failures

FAILED test/test_read_all.py::test_correct_read[read xtensor_zarr (nested) zarr using jzarr, blosc]
10268
FAILED test/test_read_all.py::test_correct_read[read xtensor_zarr (nested) zarr using jzarr, raw]
10269
FAILED test/test_read_all.py::test_correct_read[read xtensor_zarr (nested) zarr using jzarr, zlib]
10270
FAILED test/test_read_all.py::test_correct_read[read zarr (FSStore, nested) zarr using jzarr, blosc]
10271
FAILED test/test_read_all.py::test_correct_read[read zarr (FSStore, nested) zarr using jzarr, raw]
10272
FAILED test/test_read_all.py::test_correct_read[read zarr (FSStore, nested) zarr using jzarr, zlib]
10273
FAILED test/test_read_all.py::test_correct_read[read zarr (NestedDirectoryStore, nested) zarr using jzarr, blosc]
10274
FAILED test/test_read_all.py::test_correct_read[read zarr (NestedDirectoryStore, nested) zarr using jzarr, raw]
10275
FAILED test/test_read_all.py::test_correct_read[read zarr (NestedDirectoryStore, nested) zarr using jzarr, zlib]

which should be fixed once JZarr releases https://github.com/zarr-developers/zarr-python/pull/715

@grlee77: don't know if you want to hold off on this PR until then, or introduce a way to exclude/skip/tolerate that part of the matrix.

grlee77 · 2021-05-05T18:58:45Z

@grlee77: don't know if you want to hold off on this PR until then, or introduce a way to exclude/skip/tolerate that part of the matrix.

I am fine with waiting, but if you would rather merge sooner we could introduce an environment variable to enable skipping the known failures.

SabineEmbacher · 2021-06-01T04:54:31Z

Thanks for the explanation, @SabineEmbacher. I clearly misunderstood https://jzarr.readthedocs.io/en/latest/datatype.html?highlight=unsigned#data-types

Do you have a suggestion how I could write this in the documentation to avoid misinterpretation?

joshmoore · 2021-06-03T12:03:08Z

Hey @SabineEmbacher. I guess the question is if you see a way forward here. Or is this simply an incompatibility that will remain between zarr-python and jzarr?

joshmoore · 2021-06-14T11:08:08Z

Whew. 💚 @grlee77, turning this over to you. I imagine we can continue the discussion around signedness (#37 (comment)) elsewhere.

grlee77

Thanks @joshmoore, this looks good to me now.

joshmoore added 6 commits April 22, 2021 21:48

Initial data generation commit for jzarr

0046d44

Add jzarr reading test

716922f

pom cleanup

33ec3dc

Cleanup test additions

6b6b450

Add Brockmann-Consult repository

3aa27b5

Merge 'origin/master' into jzarr

2e8336f

joshmoore mentioned this pull request Apr 27, 2021

endianness character present in .zattr for single byte data types bcdev/jzarr#25

Closed

joshmoore added 2 commits April 27, 2021 09:48

Add workaround for >u1 in jzarr

1f24c8d

Handle None jzarr return in tabulation

6d02348

joshmoore mentioned this pull request Apr 27, 2021

update jzarr generate_data.sh after dtype fix #39

Open

grlee77 reviewed Apr 27, 2021

View reviewed changes

Fix retcode and properly test shape

1ad9af0

joshmoore force-pushed the jzarr branch from 7883082 to 1ad9af0 Compare April 27, 2021 19:51

use subprocess.check_output to call jzarr test script

9c980ac

grlee77 mentioned this pull request Apr 27, 2021

Implement read tests for non python implementations #25

Open

Merge pull request #1 from grlee77/subprocess-check-output

301bc4d

use subprocess.check_output to call jzarr test script

grlee77 reviewed May 4, 2021

View reviewed changes

Comment thread test/test_read_all.py

grlee77 reviewed May 4, 2021

View reviewed changes

Comment thread generate_data/jzarr/src/main/java/zarr_implementations/jzarr/App.java

joshmoore and others added 2 commits May 4, 2021 20:25

Correct filetype keys

b85deff

Co-authored-by: Gregory R. Lee <grlee77@gmail.com>

Add full test data comparison

c68abfd

Unsign returned ints

b1dc5f4

Bump to jzarr 0.3.3

4c4d2ac

grlee77 reviewed Jun 9, 2021

View reviewed changes

Comment thread generate_data/jzarr/generate_data.sh Outdated

Improve maven builds in GHA

0fd59c7

grlee77 approved these changes Jun 16, 2021

View reviewed changes

joshmoore merged commit b02f2b8 into zarr-developers:master Jun 16, 2021

joshmoore deleted the jzarr branch June 16, 2021 06:31

joshmoore mentioned this pull request Jun 16, 2021

Review signedness of jzarr #45

Open

Uh oh!

Conversation

joshmoore commented Apr 26, 2021

Uh oh!

grlee77 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

joshmoore Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

joshmoore Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

constantinpape Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

grlee77 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

grlee77 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joshmoore commented May 4, 2021

Uh oh!

grlee77 commented May 4, 2021

Uh oh!

SabineEmbacher commented May 5, 2021

Uh oh!

joshmoore commented May 5, 2021

Uh oh!

joshmoore commented May 5, 2021

Uh oh!

grlee77 commented May 5, 2021

Uh oh!

SabineEmbacher commented Jun 1, 2021

Uh oh!

joshmoore commented Jun 3, 2021

Uh oh!

Uh oh!

joshmoore commented Jun 14, 2021

Uh oh!

grlee77 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshmoore Apr 27, 2021 •

edited

Loading