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

xarray issue with reduction on not all present expected_groups #111

Closed
wants to merge 5 commits into from

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 <4441338+LunarLanding@users.noreply.github.com>
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.

None yet

2 participants