-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Load nonindex coords ahead of concat() #1551
Conversation
xarray/core/combine.py
Outdated
@@ -208,6 +219,8 @@ def _dataset_concat(datasets, dim, data_vars, coords, compat, positions): | |||
dim, coord = _calc_concat_dim_coord(dim) | |||
datasets = [as_dataset(ds) for ds in datasets] | |||
datasets = align(*datasets, join='outer', copy=False, exclude=[dim]) | |||
# TODO: compute dask coords with a single invocation of dask.compute() | |||
datasets = [_load_coords(ds) for ds in datasets] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need this if the argument coords='different'
is set. Otherwise we don't load coords to determine if they are equal or not.
xarray/core/combine.py
Outdated
@@ -208,6 +219,8 @@ def _dataset_concat(datasets, dim, data_vars, coords, compat, positions): | |||
dim, coord = _calc_concat_dim_coord(dim) | |||
datasets = [as_dataset(ds) for ds in datasets] | |||
datasets = align(*datasets, join='outer', copy=False, exclude=[dim]) | |||
# TODO: compute dask coords with a single invocation of dask.compute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be even better :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't bother as it would be a considerable complication and it would not do any good in the most common case where the "dask" variable is just a lazy load from disk
Yeah, that's OK, I just liked your TODO :)
…On Mon, Sep 4, 2017 at 1:33 AM crusaderky ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In xarray/core/combine.py
<#1551 (comment)>:
> @@ -208,6 +219,8 @@ def _dataset_concat(datasets, dim, data_vars, coords, compat, positions):
dim, coord = _calc_concat_dim_coord(dim)
datasets = [as_dataset(ds) for ds in datasets]
datasets = align(*datasets, join='outer', copy=False, exclude=[dim])
+ # TODO: compute dask coords with a single invocation of dask.compute()
I didn't bother as it would be a considerable complication and it would
not do any good in the most common case where the "dask" variable is just a
lazy load from disk
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1551 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABKS1kdcqAVVgdHoOZGKBf-IuFBdVJWbks5se7XugaJpZM4PLCZy>
.
|
I think this should help with #1385. |
New, much better version. |
Fix loads when vars are found different halfway through
Finished. One more test failure, as it now triggers #1588 too. |
@shoyer ready to merge if there's no outstanding critiques |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Let's merge after syncing the latest changes from master.
There is still room for improving the logic in concat but this is a clear step forward.
xarray/tests/test_combine.py
Outdated
@@ -268,6 +269,7 @@ def test_concat(self): | |||
with self.assertRaisesRegexp(ValueError, 'not a valid argument'): | |||
concat([foo, bar], dim='w', data_vars='minimal') | |||
|
|||
@pytest.mark.xfail(reason='https://github.com/pydata/xarray/issues/1586') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just merged your other PR -- can you remove this now?
@shoyer ready for merge |
git diff upstream/master | flake8 --diff
whats-new.rst
for all changes andapi.rst
for new API