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

Add hypothesis property tests #1746

Merged
merged 26 commits into from
Aug 8, 2024
Merged

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Apr 5, 2024

After mamba install hypothesis

Use

python -m pytest --capture=no --hypothesis-verbosity=verbose test_properties.py

to see all the things it tries.

This was a quick attempt at a property test.

The other thing you can do is a "Stateful" test (e.g. https://github.com/pydata/xarray/blob/main/properties/test_index_manipulation.py)

which runs an arbitrary sequence of manipulations (e.g. add array, rename, move, delete, modify array, copy), and checks for consistency at each point.

cc @Zac-HD

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@pep8speaks
Copy link

pep8speaks commented Apr 5, 2024

Hello @dcherian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 5:1: E402 module level import not at top of file
Line 6:1: E402 module level import not at top of file
Line 8:1: E402 module level import not at top of file
Line 9:1: E402 module level import not at top of file
Line 10:1: E402 module level import not at top of file
Line 11:1: E402 module level import not at top of file
Line 12:1: E402 module level import not at top of file
Line 14:1: E402 module level import not at top of file
Line 15:1: E402 module level import not at top of file
Line 17:1: E266 too many leading '#' for block comment

Comment last updated at 2024-04-05 17:47:12 UTC

Comment on lines 77 to 92
elif path == "/":
assert name is not None
array_path = name
array_name = "/" + name
else:
assert name is not None
array_path = f"{path}/{name}"
array_name = "/" + array_path
Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally not obvious!

tests/test_properties.py Outdated Show resolved Hide resolved
1. Roundtrip a numpy array
2. Basic Indexing
@dcherian
Copy link
Contributor Author

dcherian commented Jun 5, 2024

Some input needed:

  1. Should this be a separate action or folded in to the existing actions? I lean former. It's fast now (47s) but it could get slower in the future.
  2. There's more to make strategies.py actually useful as public API but I'd like to punt that to the future.
  3. I'll make comments where I'd like a careful review.

src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
Comment on lines 74 to 75
# TODO: clean this up
if path is None and name is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this mess be cleaned up?

src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
* v3: (22 commits)
  [v3] `Buffer` ensure correct subclass based on the `BufferPrototype` argument (zarr-developers#1974)
  Fix doc build (zarr-developers#1987)
  Fix doc build warnings (zarr-developers#1985)
  Automatically generate API reference docs (zarr-developers#1918)
  Update `RemoteStore.__str__` and add UPath tests (zarr-developers#1964)
  [v3] Elevate codec pipeline (zarr-developers#1932)
  0 dim arrays: indexing (zarr-developers#1980)
  `parse_shapelike` allows 0 (zarr-developers#1979)
  Clean up typing and docs for indexing (zarr-developers#1961)
  add json indentation to config (zarr-developers#1952)
  chore: update pre-commit hooks (zarr-developers#1973)
  Bump pypa/gh-action-pypi-publish in the actions group (zarr-developers#1969)
  chore: update pre-commit hooks (zarr-developers#1957)
  Update release.rst (zarr-developers#1960)
  doc: update release notes for 3.0.0.alpha (zarr-developers#1959)
  Basic working FsspecStore (zarr-developers#1785)
  Feature: Top level V3 API (zarr-developers#1884)
  Buffer Prototype Argument (zarr-developers#1910)
  Create issue-metrics.yml
  fixes bug in transpose (zarr-developers#1949)
  ...
* v3:
  Allow 'chunks' as an alias for 'chunk_shape' in array creation. (zarr-developers#1991)
@joshmoore joshmoore added the V3 Related to compatibility with V3 spec label Jul 11, 2024
* v3: (22 commits)
  chore: update pre-commit hooks (zarr-developers#2051)
  Apply ruff/flake8-bandit rule B006 (zarr-developers#2049)
  Move fixtures to `tests` (zarr-developers#1813)
  Multiple imports for an import name (zarr-developers#2047)
  Redundant list comprehension (zarr-developers#2048)
  chore: update pre-commit hooks (zarr-developers#2039)
  Cast fill value to array's dtype (zarr-developers#2020)
  chore: update pre-commit hooks (zarr-developers#2017)
  make shardingcodec pickleable  (zarr-developers#2011)
  doc: copy 3.0.0.alpha changelog into release.rst (zarr-developers#2007)
  build(ci): enable python 3.12 in github actions (zarr-developers#2005)
  Bump NumPy to 2.0 (zarr-developers#1983)
  chore: update pre-commit hooks (zarr-developers#1989)
  Fix indexing with bools (zarr-developers#1968)
  Fix string interpolation (zarr-developers#1998)
  Unnecessary comprehension (zarr-developers#1997)
  Stop ignoring these ruff rules (zarr-developers#2001)
  Merge collapsible if statements (zarr-developers#1999)
  Unnecessary comprehension (zarr-developers#1996)
  Handle Path in `make_store_path` (zarr-developers#1992)
  ...
@dcherian dcherian changed the title Add example hypothesis roundtrip test Add hypothesis property tests Jul 25, 2024
@dcherian dcherian marked this pull request as ready for review July 25, 2024 03:31
@dcherian dcherian requested a review from jhamman July 25, 2024 03:32
@dcherian
Copy link
Contributor Author

dcherian commented Aug 5, 2024

This looks like a real failure:

 E           AssertionError: 
E           Arrays are not equal
E           
E           Mismatched elements: 1 / 1 (100%)
E           Max absolute difference: 1
E           Max relative difference: inf
E            x: array([1], dtype=int8)
E            y: array([0], dtype=int8)

Apparently it is not roundtripping np.array([1], dtype=np.int8) @d-v-b are you able to take a look?

@d-v-b
Copy link
Contributor

d-v-b commented Aug 5, 2024

This looks like a real failure:

 E           AssertionError: 
E           Arrays are not equal
E           
E           Mismatched elements: 1 / 1 (100%)
E           Max absolute difference: 1
E           Max relative difference: inf
E            x: array([1], dtype=int8)
E            y: array([0], dtype=int8)

Apparently it is not roundtripping np.array([1], dtype=np.int8) @d-v-b are you able to take a look?

definitely! Given that I know nothing about hypothesis, how can I replicate this failure locally?

@dcherian
Copy link
Contributor Author

dcherian commented Aug 5, 2024

This may be user error. It's sensitive to the store.close()

import zarr
from zarr.array import Array
from zarr.group import Group
from zarr.store import MemoryStore
import numpy as np

store = MemoryStore(mode="w")
root = Group.create(store)
nparray = np.array([1], dtype=np.int8)
    
a = root.create_array(
    "/0/0",
    shape=nparray.shape,
    chunks=(1,),
    dtype=nparray.dtype.str,
    attributes={},
    # compressor=compressor,  # TODO: FIXME
    fill_value=nparray.dtype.type(0),
)
a[:] = nparray
print(a[:]) # [1]
store.close()
print(a[:]) # [0]

@d-v-b
Copy link
Contributor

d-v-b commented Aug 5, 2024

I can replicate this locally. store.close is quite minimal so I think our problem lies elsewhere. I will investigate.

src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
src/zarr/strategies.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor Author

dcherian commented Aug 8, 2024

pre-commit.ci autofix

@jhamman jhamman merged commit 334d6fe into zarr-developers:v3 Aug 8, 2024
25 checks passed
@dcherian dcherian deleted the hypothesis-tests branch August 8, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V3 Related to compatibility with V3 spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants