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

Stateful tests with Dataset #8658

Merged
merged 72 commits into from
Apr 3, 2024
Merged

Stateful tests with Dataset #8658

merged 72 commits into from
Apr 3, 2024

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented Jan 24, 2024

I was curious to see if the hypothesis stateful testing would catch an inconsistent sequence of index manipulation operations like #8646. Turns out rename_vars is basically broken? (filed #8659) :P

PS: this blog post is amazing.

E           state = DatasetStateMachine()
E           state.assert_invariants()
E           > ===
E
E            <xarray.Dataset>
E           Dimensions:  ()
E           Data variables:
E               *empty*
E           ===
E
E
E           > vars: ('1', '1_')
E           state.add_dim_coord(var=<xarray.Variable (1: 1)>
E           array([0], dtype=uint32))
E           state.assert_invariants()
E           > ===
E
E            <xarray.Dataset>
E           Dimensions:  (1: 1)
E           Coordinates:
E             * 1        (1) uint32 0
E           Data variables:
E               1_       (1) uint32 0
E           ===
E
E
E           > renaming 1 to 0
E           state.rename_vars(newname='0')
E           state.assert_invariants()
E           > ===
E
E            <xarray.Dataset>
E           Dimensions:  (1: 1)
E           Coordinates:
E             * 0        (1) uint32 0
E           Dimensions without coordinates: 1
E           Data variables:
E               1_       (1) uint32 0
E           ===
E
E
E           state.teardown()

@TomNicholas
Copy link
Member

Oh this is a really cool idea!

Also yeah I've read that blog post before and my mind was also blown haha

@TomNicholas TomNicholas added topic-hypothesis Strategies or tests using the hypothesis library topic-testing labels Jan 24, 2024
@max-sixty
Copy link
Collaborator

Great idea! Really cool.

I even wonder to what extent this could replace vast swathes of our tests — anything where the output is calculated from the input.

We'll always want some unit tests to code to when developing, but plausibly the unit tests are either a) examples or b) some saved cases that have failed these model-based tests before.

@benbovy
Copy link
Member

benbovy commented Jan 25, 2024

That looks very cool indeed! (although I don't think that rename_vars is broken :) but instead some valid cases require skipping the default indexes invariant check).

@dcherian
Copy link
Contributor Author

instead some valid cases require skipping the default indexes invariant check.

Can we write down rules for when these checks are needed?

@benbovy
Copy link
Member

benbovy commented Jan 26, 2024

Yes although I think it is easier to write rules for when these checks aren't needed.

One example: skip the default indexes invariant test when the name of an existing dimension coordinate is passed as input kwarg or dict key to .rename_vars().

dcherian added 22 commits March 9, 2024 10:25
* main: (31 commits)
  correctly encode/decode _FillValues/missing_values/dtypes for packed data (pydata#8713)
  Expand use of `.oindex` and `.vindex` (pydata#8790)
  Return a dataclass from Grouper.factorize (pydata#8777)
  [skip-ci] Fix upstream-dev env (pydata#8839)
  Add dask-expr for windows envs (pydata#8837)
  [skip-ci] Add dask-expr dependency to doc.yml (pydata#8835)
  Add `dask-expr` to environment-3.12.yml (pydata#8827)
  Make list_chunkmanagers more resilient to broken entrypoints (pydata#8736)
  Do not attempt to broadcast when global option ``arithmetic_broadcast=False`` (pydata#8784)
  try to get the `upstream-dev` CI to complete again (pydata#8823)
  Bump the actions group with 1 update (pydata#8818)
  Update documentation for clarity (pydata#8817)
  DOC: link to zarr.convenience.consolidate_metadata (pydata#8816)
  Refactor Grouper objects (pydata#8776)
  Grouper object design doc (pydata#8510)
  Bump the actions group with 2 updates (pydata#8804)
  tokenize() should ignore difference between None and {} attrs (pydata#8797)
  fix: remove Coordinate from __all__ in xarray/__init__.py (pydata#8791)
  Fix non-nanosecond casting behavior for `expand_dims` (pydata#8782)
  Migrate treenode module. (pydata#8757)
  ...
@dcherian dcherian force-pushed the state-machine branch 2 times, most recently from 7d796ee to 0f76396 Compare April 2, 2024 00:01
@dcherian dcherian added the run-slow-hypothesis Run slow hypothesis tests label Apr 2, 2024
@dcherian dcherian force-pushed the state-machine branch 2 times, most recently from 63d3c47 to b9433b6 Compare April 2, 2024 00:10
@dcherian dcherian marked this pull request as ready for review April 2, 2024 01:49
@dcherian
Copy link
Contributor Author

dcherian commented Apr 2, 2024

Hah now our strategy tests are failing

dcherian added 4 commits April 1, 2024 20:05
* main: (26 commits)
  [pre-commit.ci] pre-commit autoupdate (pydata#8900)
  Bump the actions group with 1 update (pydata#8896)
  New empty whatsnew entry (pydata#8899)
  Update reference to 'Weighted quantile estimators' (pydata#8898)
  2024.03.0: Add whats-new (pydata#8891)
  Add typing to test_groupby.py (pydata#8890)
  Avoid in-place multiplication of a large value to an array with small integer dtype (pydata#8867)
  Check for aligned chunks when writing to existing variables (pydata#8459)
  Add dt.date to plottable types (pydata#8873)
  Optimize writes to existing Zarr stores. (pydata#8875)
  Allow multidimensional variable with same name as dim when constructing dataset via coords (pydata#8886)
  Don't allow overwriting indexes with region writes (pydata#8877)
  Migrate datatree.py module into xarray.core. (pydata#8789)
  warn and return bytes undecoded in case of UnicodeDecodeError in h5netcdf-backend (pydata#8874)
  groupby: Dispatch quantile to flox. (pydata#8720)
  Opt out of auto creating index variables (pydata#8711)
  Update docs on view / copies (pydata#8744)
  Handle .oindex and .vindex for the PandasMultiIndexingAdapter and PandasIndexingAdapter (pydata#8869)
  numpy 2.0 copy-keyword and trapz vs trapezoid (pydata#8865)
  upstream-dev CI: Fix interp and cumtrapz (pydata#8861)
  ...
@dcherian dcherian requested a review from TomNicholas April 2, 2024 03:29
Copy link
Contributor

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Obviously there are some remaining todo comments, but this looks great! I'd consider merging it more or less as-is, and then continuing work in a follow-up PR 🙂

Comment on lines +63 to +65
# Can't use bundles because we'd need pre-conditions on consumes(bundle)
# indexed_dims = Bundle("indexed_dims")
# multi_indexed_dims = Bundle("multi_indexed_dims")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think HypothesisWorks/hypothesis#3944 will enable this, though I wouldn't delay this PR to wait.

import xarray as xr

ds = xr.Dataset()
ds["0"] = np.array(["", "\x000"], dtype=object)
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem that surprising that you hit bugs related to empty-string or null-prefixed string? To most C code, those are identical!

xarray/testing/strategies.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor Author

dcherian commented Apr 3, 2024

Thanks for the review @Zac-HD

@dcherian dcherian enabled auto-merge (squash) April 3, 2024 21:09
@dcherian dcherian disabled auto-merge April 3, 2024 21:29
@dcherian dcherian merged commit 40afd30 into pydata:main Apr 3, 2024
30 of 31 checks passed
@dcherian dcherian deleted the state-machine branch April 3, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-slow-hypothesis Run slow hypothesis tests topic-hypothesis Strategies or tests using the hypothesis library topic-testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants