Skip to content

Conversation

@LunarLanding
Copy link
Contributor

The test below fails before this commit and succeeds after.
However getting the correct fill_value, if any, and avoiding computation so the current tests do not fail will require further work.

def test_fix_xarray():
    import flox.xarray
    import xarray as xr
    import numpy as np

    sa=10
    sb=13
    sc=3

    x=xr.Dataset({
        'v0':xr.DataArray(((np.arange(sa*sb*sc)/sa)%1).reshape((sa,sb,sc)),dims=('a','b','c')),
        'v1':xr.DataArray((np.arange(sa*sb)%3).reshape(sa,sb),dims=('a','b')),
    })

    r=flox.xarray.xarray_reduce(x['v0'],
                              x['v1'],x['v0'],
                              expected_groups=(np.arange(6),np.linspace(0,1,num=5)),
                              isbin=[False,True],
                              func='count',
                              dim='b',
                              fill_value=np.nan,
                             ).mean('a',skipna=True)
    assert len(r['v1'])==6
test_fix_xarray()

dcherian added a commit that referenced this pull request Jun 23, 2022
This affected _factorize_multiple.

Closes #111
@dcherian
Copy link
Collaborator

Thanks @LunarLanding for the test and attempting to fix it. I have a simpler fix in #112. Can you try that and see if it looks like expected?

@LunarLanding
Copy link
Contributor Author

LunarLanding commented Jun 23, 2022

Oh my, it is much simpler, thanks! I found an issue when I started testing the dask case, fixed it like this LunarLanding@10996a8 , tests run ok.

@dcherian
Copy link
Collaborator

Oh nice catch. Can you open a pull request to that branch? That way, you can get some credit too

@LunarLanding LunarLanding deleted the fix/xarray branch June 23, 2022 21:18
dcherian added a commit that referenced this pull request Jun 23, 2022
* Fix bug where we had extra groups in expected_groups.

This affected _factorize_multiple.

Closes #111

* Fix extra expected groups (#113)

* fix dask case

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Co-authored-by: LunarLanding <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants