Fix order handling

Author: dstansby

changes/xxx1.bugfix.rst

@@ -0,0 +1 @@
+Fixed the error message when passing both ``config`` and ``write_emtpy_chunks`` arguments to reflect the current behaviour (``write_empty_chunks`` takes precedence).

changes/xxx2.bugfix.rst

@@ -0,0 +1 @@
+Creating a Zarr format 2 array with the ``order`` keyword argument no longer raises a warning.

changes/xxx3.bugfix.rst

@@ -0,0 +1 @@
+Creating a Zarr format 3 array with the ``order`` argument now conistently ignores this argument and raises a warning.

changes/xxx4.bugfix.rst

@@ -0,0 +1,2 @@
+When using ``from_array`` to copy a Zarr format 2 array to a Zarr format 3 array, if the memory order of the input array is ``"F"`` a warning is raised and the order ignored.
+This is because Zarr format 3 arrays are always stored in "C" order.

changes/xxxx.bugfix.rst

@@ -0,0 +1 @@
+The ``config`` argument to `zarr.create` (and functions that create arrays) is now used - previously it had no effect.

src/zarr/api/asynchronous.py

@@ -17,7 +17,7 @@
     from_array,
     get_array_metadata,
 )
-from zarr.core.array_spec import ArrayConfig, ArrayConfigLike, ArrayConfigParams
+from zarr.core.array_spec import ArrayConfigLike, parse_array_config
 from zarr.core.buffer import NDArrayLike
 from zarr.core.common import (
     JSON,
@@ -27,7 +27,6 @@
     MemoryOrder,
     ZarrFormat,
     _default_zarr_format,
-    _warn_order_kwarg,
     _warn_write_empty_chunks_kwarg,
     parse_dtype,
 )
@@ -1013,8 +1012,6 @@ async def create(
         warnings.warn("object_codec is not yet implemented", RuntimeWarning, stacklevel=2)
     if read_only is not None:
         warnings.warn("read_only is not yet implemented", RuntimeWarning, stacklevel=2)
-    if order is not None:
-        _warn_order_kwarg()
     if write_empty_chunks is not None:
         _warn_write_empty_chunks_kwarg()
 
@@ -1026,26 +1023,17 @@ async def create(
         mode = "a"
     store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options)
 
-    config_dict: ArrayConfigParams = {}
+    config_parsed = parse_array_config(config)
 
     if write_empty_chunks is not None:
         if config is not None:
             msg = (
                 "Both write_empty_chunks and config keyword arguments are set. "
-                "This is redundant. When both are set, write_empty_chunks will be ignored and "
-                "config will be used."
+                "This is redundant. When both are set, write_empty_chunks will be used instead "
+                "of the value in config."
             )
             warnings.warn(UserWarning(msg), stacklevel=1)
-        config_dict["write_empty_chunks"] = write_empty_chunks
-    if order is not None and config is not None:
-        msg = (
-            "Both order and config keyword arguments are set. "
-            "This is redundant. When both are set, order will be ignored and "
-            "config will be used."
-        )
-        warnings.warn(UserWarning(msg), stacklevel=1)
-
-    config_parsed = ArrayConfig.from_dict(config_dict)
+        config_parsed = dataclasses.replace(config_parsed, write_empty_chunks=write_empty_chunks)
 
     return await AsyncArray._create(
         store_path,
@@ -1248,8 +1236,6 @@ async def open_array(
 
     zarr_format = _handle_zarr_version_or_format(zarr_version=zarr_version, zarr_format=zarr_format)
 
-    if "order" in kwargs:
-        _warn_order_kwarg()
     if "write_empty_chunks" in kwargs:
         _warn_write_empty_chunks_kwarg()
 

src/zarr/core/array.py

@@ -62,7 +62,6 @@
     _warn_order_kwarg,
     concurrent_map,
     parse_dtype,
-    parse_order,
     parse_shapelike,
     product,
 )
@@ -603,7 +602,6 @@ async def _create(
 
             if order is not None:
                 _warn_order_kwarg()
-                config_parsed = replace(config_parsed, order=order)
 
             result = await cls._create_v3(
                 store_path,
@@ -631,9 +629,10 @@ async def _create(
                 raise ValueError("dimension_names cannot be used for arrays with zarr_format 2.")
 
             if order is None:
-                order_parsed = parse_order(zarr_config.get("array.order"))
+                order_parsed = config_parsed.order
             else:
                 order_parsed = order
+                config_parsed = replace(config_parsed, order=order)
 
             result = await cls._create_v2(
                 store_path,
@@ -721,6 +720,7 @@ async def _create_v3(
         attributes: dict[str, JSON] | None = None,
         overwrite: bool = False,
     ) -> AsyncArray[ArrayV3Metadata]:
+        print(config)
         if overwrite:
             if store_path.store.supports_deletes:
                 await store_path.delete_dir()
@@ -4026,6 +4026,7 @@ async def from_array(
         chunk_key_encoding=chunk_key_encoding,
         dimension_names=dimension_names,
     )
+    print(order)
     if not hasattr(data, "dtype") or not hasattr(data, "shape"):
         data = np.array(data)
 
@@ -4267,10 +4268,8 @@ async def init_array(
             chunks_out = chunk_shape_parsed
             codecs_out = sub_codecs
 
-        if config is None:
-            config = {}
-        if order is not None and isinstance(config, dict):
-            config["order"] = config.get("order", order)
+        if order is not None:
+            _warn_order_kwarg()
 
         meta = AsyncArray._create_metadata_v3(
             shape=shape_parsed,
@@ -4521,15 +4520,26 @@ def _parse_keep_array_attr(
                 serializer = "auto"
         if fill_value is None:
             fill_value = data.fill_value
-        if order is None:
+
+        if data.metadata.zarr_format == 2 and zarr_format == 3 and data.order == "F":
+            # Can't set order="F" for v3 arrays
+            warnings.warn(
+                "Zarr format 3 arrays are always stored with order='C'. "
+                "The existing order='F' of the source Zarr format 2 array will be ignored.",
+                UserWarning,
+                stacklevel=2,
+            )
+        elif order is None and zarr_format == 2:
             order = data.order
+
         if chunk_key_encoding is None and zarr_format == data.metadata.zarr_format:
             if isinstance(data.metadata, ArrayV2Metadata):
                 chunk_key_encoding = {"name": "v2", "separator": data.metadata.dimension_separator}
             elif isinstance(data.metadata, ArrayV3Metadata):
                 chunk_key_encoding = data.metadata.chunk_key_encoding
         if dimension_names is None and data.metadata.zarr_format == 3:
             dimension_names = data.metadata.dimension_names
+        print(order)
     else:
         if chunks == "keep":
             chunks = "auto"

src/zarr/core/group.py

@@ -2417,7 +2417,7 @@ def create_array(
         compressor: CompressorLike = "auto",
         serializer: SerializerLike = "auto",
         fill_value: Any | None = 0,
-        order: MemoryOrder | None = "C",
+        order: MemoryOrder | None = None,
         attributes: dict[str, JSON] | None = None,
         chunk_key_encoding: ChunkKeyEncodingLike | None = None,
         dimension_names: DimensionNames = None,
@@ -2811,7 +2811,7 @@ def array(
         compressor: CompressorLike = None,
         serializer: SerializerLike = "auto",
         fill_value: Any | None = 0,
-        order: MemoryOrder | None = "C",
+        order: MemoryOrder | None = None,
         attributes: dict[str, JSON] | None = None,
         chunk_key_encoding: ChunkKeyEncodingLike | None = None,
         dimension_names: DimensionNames = None,

test.py

@@ -0,0 +1,11 @@
+import zarr
+
+# Write fortran style array
+group = zarr.group(store={})
+array = group.create_array(
+    name="example",
+    shape=(128, 128),
+    dtype="float32",
+    order="F",
+    dimension_names=("row", "col"),
+)

tests/test_api.py

@@ -339,34 +339,49 @@ def test_open_with_mode_w_minus(tmp_path: pathlib.Path) -> None:
         zarr.open(store=tmp_path, mode="w-")
 
 
-def test_array_order(zarr_format: ZarrFormat) -> None:
-    arr = zarr.ones(shape=(2, 2), order=None, zarr_format=zarr_format)
-    expected = zarr.config.get("array.order")
-    assert arr.order == expected
-
-    vals = np.asarray(arr)
-    if expected == "C":
-        assert vals.flags.c_contiguous
-    elif expected == "F":
-        assert vals.flags.f_contiguous
-    else:
-        raise AssertionError
+@pytest.mark.parametrize("order", ["C", "F", None])
+@pytest.mark.parametrize("config", [{"order": "C"}, {"order": "F"}, {}], ids=["C", "F", "None"])
+def test_array_order(
+    order: MemoryOrder, config: dict[str, MemoryOrder], zarr_format: ZarrFormat
+) -> None:
+    """
+    Check that:
+    - For v2, memory order is taken from the `order` keyword argument.
+    - For v3, memory order is taken from `config`, and when order is passed a warning is raised
+    - The numpy array returned has the expected order
+    - For v2, the order metadata is set correctly
+    """
+    default_order = zarr.config.get("array.order")
+    if zarr_format == 3:
+        if order is not None:
+            ctx = pytest.warns(
+                RuntimeWarning,
+                match="The `order` keyword argument has no effect for Zarr format 3 arrays",
+            )
+        else:
+            ctx = contextlib.nullcontext()
+        expected_order = config.get("order", default_order)
 
+    if zarr_format == 2:
+        ctx = contextlib.nullcontext()
+        expected_order = order or config.get("order", default_order)
 
-@pytest.mark.parametrize("order", ["C", "F"])
-def test_array_order_warns(order: MemoryOrder | None, zarr_format: ZarrFormat) -> None:
-    with pytest.warns(RuntimeWarning, match="The `order` keyword argument .*"):
-        arr = zarr.ones(shape=(2, 2), order=order, zarr_format=zarr_format)
-    assert arr.order == order
+    with ctx:
+        arr = zarr.ones(shape=(2, 2), order=order, zarr_format=zarr_format, config=config)
 
+    assert arr.order == expected_order
     vals = np.asarray(arr)
-    if order == "C":
+    if expected_order == "C":
         assert vals.flags.c_contiguous
-    elif order == "F":
+    elif expected_order == "F":
         assert vals.flags.f_contiguous
     else:
         raise AssertionError
 
+    if zarr_format == 2:
+        assert arr.metadata.zarr_format == 2
+        assert arr.metadata.order == expected_order
+
 
 # def test_lazy_loader():
 #     foo = np.arange(100)

tests/test_array.py

@@ -6,7 +6,7 @@
 import re
 import sys
 from itertools import accumulate
-from typing import TYPE_CHECKING, Any, Literal
+from typing import Any, Literal
 from unittest import mock
 
 import numcodecs
@@ -42,18 +42,15 @@
 from zarr.core.buffer.cpu import NDBuffer
 from zarr.core.chunk_grids import _auto_partition
 from zarr.core.chunk_key_encodings import ChunkKeyEncodingParams
-from zarr.core.common import JSON, MemoryOrder, ZarrFormat
+from zarr.core.common import JSON, ZarrFormat
 from zarr.core.group import AsyncGroup
 from zarr.core.indexing import BasicIndexer, ceildiv
+from zarr.core.metadata.v2 import ArrayV2Metadata
 from zarr.core.metadata.v3 import ArrayV3Metadata, DataType
 from zarr.core.sync import sync
 from zarr.errors import ContainsArrayError, ContainsGroupError
 from zarr.storage import LocalStore, MemoryStore, StorePath
 
-if TYPE_CHECKING:
-    from zarr.core.array_spec import ArrayConfigLike
-from zarr.core.metadata.v2 import ArrayV2Metadata
-
 
 @pytest.mark.parametrize("store", ["local", "memory", "zip"], indirect=["store"])
 @pytest.mark.parametrize("zarr_format", [2, 3])
@@ -1400,52 +1397,6 @@ async def test_data_ignored_params(store: Store) -> None:
         ):
             await create_array(store, data=data, shape=None, dtype=data.dtype, overwrite=True)
 
-    @staticmethod
-    @pytest.mark.parametrize("order", ["C", "F", None])
-    @pytest.mark.parametrize("with_config", [True, False])
-    def test_order(
-        order: MemoryOrder | None,
-        with_config: bool,
-        zarr_format: ZarrFormat,
-        store: MemoryStore,
-    ) -> None:
-        """
-        Test that the arrays generated by array indexing have a memory order defined by the config order
-        value, and that for zarr v2 arrays, the ``order`` field in the array metadata is set correctly.
-        """
-        config: ArrayConfigLike | None = {}
-        if order is None:
-            config = {}
-            expected = zarr.config.get("array.order")
-        else:
-            config = {"order": order}
-            expected = order
-
-        if not with_config:
-            # Test without passing config parameter
-            config = None
-
-        arr = zarr.create_array(
-            store=store,
-            shape=(2, 2),
-            zarr_format=zarr_format,
-            dtype="i4",
-            order=order,
-            config=config,
-        )
-        assert arr.order == expected
-        if zarr_format == 2:
-            assert arr.metadata.zarr_format == 2
-            assert arr.metadata.order == expected
-
-        vals = np.asarray(arr)
-        if expected == "C":
-            assert vals.flags.c_contiguous
-        elif expected == "F":
-            assert vals.flags.f_contiguous
-        else:
-            raise AssertionError
-
     @staticmethod
     @pytest.mark.parametrize("write_empty_chunks", [True, False])
     async def test_write_empty_chunks_config(write_empty_chunks: bool, store: Store) -> None:

tests/test_metadata/test_consolidated.py

@@ -618,6 +618,7 @@ async def test_use_consolidated_for_children_members(
         expected = ["b", "b/c"]
         assert result == expected
 
+
 @pytest.mark.parametrize("fill_value", [np.nan, np.inf, -np.inf])
 async def test_consolidated_metadata_encodes_special_chars(
     memory_store: Store, zarr_format: ZarrFormat, fill_value: float