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.

Sign up for GitHub

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

Jump to bottom

Hotfix for #2662 #2678

Merged
merged 4 commits into from
Jan 17, 2019
Merged

Hotfix for #2662 #2678

merged 4 commits into from
Jan 17, 2019

Conversation

TomNicholas
Copy link
Member

Explained in #2662. Also renamed some variables slightly for clarity.

Not sure how to add a test without refactoring the groupby into a separate function, as in it's current form the problem only manifests as a (huge) slowdown.

@malmans2 malmans2 mentioned this pull request Jan 15, 2019
Closed
@TomNicholas
Copy link
Member Author

This only crept through the tests because merge was forgiving with what it would merge - should we change the arguments to merge in auto_combine to be something like compat='no_conflicts' so that it fails out instead?

@shoyer
Copy link
Member

shoyer commented Jan 16, 2019

This only crept through the tests because merge was forgiving with what it would merge - should we change the arguments to merge in auto_combine to be something like compat='no_conflicts' so that it fails out instead?

We could need compat='equals' to be stricter, but I suspect that some some users do rely on this "no conflicts" behavior.

This might be a good idea for tests, though.

shoyer
shoyer reviewed Jan 16, 2019
View reviewed changes
xarray/core/combine.py Outdated Show resolved Hide resolved
shoyer
shoyer reviewed Jan 17, 2019
View reviewed changes
xarray/tests/test_combine.py Outdated
actual = auto_combine(objs, concat_dim=[None, 'x'], compat='equals')
assert_identical(expected, actual)

def test_internal_odering(self):
Copy link
Member

Choose a reason for hiding this comment

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

Does this test fail with the old logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it gives a MergeError.

Though maybe I need to dig more, because I thought it would pass with compat='no_conflicts', and fail with compat='equals'...

@shoyer shoyer merged commit dc87dea into pydata:master Jan 17, 2019
@shoyer
Copy link
Member

shoyer commented Jan 17, 2019

thanks @TomNicholas !

dcherian pushed a commit to dcherian/xarray that referenced this pull request Jan 24, 2019
Merge branch 'master' into refactor-plot-utils
43057ef 
* master:
  stale requires a label (pydata#2701)
  Update indexing.rst (pydata#2700)
  add line break to message posted (pydata#2698)
  Config for closing stale issues (pydata#2684)
  to_dict without data (pydata#2659)
  Update asv.conf.json (pydata#2693)
  try no rasterio in py36 env (pydata#2691)
  Detailed report for testing.assert_equal and testing.assert_identical (pydata#1507)
  Hotfix for pydata#2662 (pydata#2678)
  Update README.rst (pydata#2682)
  Fix test failures with numpy=1.16 (pydata#2675)
dcherian pushed a commit to yohai/xarray that referenced this pull request Jan 24, 2019
Merge branch 'refactor-plot-utils' into yohai-ds_scatter
57a6c64 
* refactor-plot-utils: (22 commits)
  review comment.
  small rename
  stale requires a label (pydata#2701)
  Update indexing.rst (pydata#2700)
  add line break to message posted (pydata#2698)
  Config for closing stale issues (pydata#2684)
  to_dict without data (pydata#2659)
  Update asv.conf.json (pydata#2693)
  try no rasterio in py36 env (pydata#2691)
  Detailed report for testing.assert_equal and testing.assert_identical (pydata#1507)
  Hotfix for pydata#2662 (pydata#2678)
  Update README.rst (pydata#2682)
  Fix test failures with numpy=1.16 (pydata#2675)
  lint
  Back to map_dataarray_line
  Refactor out cmap_params, cbar_kwargs processing
  Refactor out colorbar making to plot.utils._add_colorbar
  flake8
  facetgrid refactor
  Refactor out utility functions.
  ...
shoyer pushed a commit that referenced this pull request Jan 24, 2019
@TomNicholas @shoyer
Hotfix for #2662 (#2678)
55fbd41 
* Hotfix for #2662

* Separate keyfunc

* Added a test, and made others slightly stricter

* Comment explaining test better
@TomNicholas TomNicholas deleted the hotfix_for_2662 branch February 2, 2019 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoyer shoyer shoyer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TomNicholas @shoyer