Add rename_vars and rename_dims #3045

jukent · 2019-06-25T22:50:33Z

Closes Rename dims independently from coords? #3026
Tests added
Fully documented, including whats-new.rst for all changes and api.rst for new API

jukent · 2019-06-25T22:50:51Z

I uninstalled the bot that automatically closed the pull request #3042

pep8speaks · 2019-06-25T22:50:51Z

Hello @jukent! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-02 18:28:57 UTC

xarray/tests/test_dataset.py

max-sixty · 2019-06-25T23:12:36Z

FYI the failure in appveyor is unrelated and can be ignored

xarray/tests/test_dataset.py

xarray.code-workspace

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

max-sixty

Great! Thanks @jukent

Only items:

remove miscreant (vscode?) file
depending on your + others thoughts, change to rename_variables
add kwargs test

xarray/core/dataset.py

xarray/tests/test_dataset.py

xarray/core/dataset.py

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

dcherian · 2019-06-26T17:54:37Z

@jukent You should also give yourself credit in whats-new.rst and add stuff to api.rst. I also see that you're a first-time contributor. Welcome! We really appreciate your efforts.

xarray/core/dataset.py

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

max-sixty · 2019-06-26T18:43:08Z

FYI tests are failing with E NameError: name '_rename_var_dims_helper' is not defined: https://travis-ci.org/pydata/xarray/jobs/550940568

jukent · 2019-06-26T19:17:14Z

FYI tests are failing with E NameError: name '_rename_var_dims_helper' is not defined: https://travis-ci.org/pydata/xarray/jobs/550940568

Huh not sure why that is. I can remove that function and see if it works then.

shoyer · 2019-06-26T21:38:17Z

I would definitely recommend reading through our contributor's guide on how to setup a local environment for testing your code: http://xarray.pydata.org/en/stable/contributing.html

It is much easier to fix issues when you run tests locally rather than waiting on continuous integration tests (Travis-CI) to tell you that things are broken.

jukent · 2019-06-26T22:39:31Z

I would definitely recommend reading through our contributor's guide on how to setup a local environment for testing your code: http://xarray.pydata.org/en/stable/contributing.html

It is much easier to fix issues when you run tests locally rather than waiting on continuous integration tests (Travis-CI) to tell you that things are broken.

Thank you was just needing to look into this.

jukent · 2019-07-01T17:14:08Z

Checks could be more complicated, but I think this is ready to merge!

dcherian

whats-new fix required. Otherwise LGTM.

doc/whats-new.rst

codecov · 2019-07-01T21:41:52Z

Codecov Report

Merging #3045 into master will increase coverage by 1.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #3045      +/-   ##
==========================================
+ Coverage   93.78%   94.79%   +1.01%     
==========================================
  Files          68       67       -1     
  Lines       13156    12892     -264     
==========================================
- Hits        12338    12221     -117     
+ Misses        818      671     -147

Impacted Files	Coverage Δ
xarray/core/dataset.py	`96.14% <94.11%> (-0.1%)`	⬇️
xarray/_version.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aeee7f...b3232ad. Read the comment docs.

dcherian · 2019-07-01T22:03:23Z

OK fixed whats-new. Sorry it gets a little confusing if there's been a release in the meantime.

This looks good to me. @max-sixty check once more and merge?

shoyer · 2019-07-02T01:59:11Z

Looking at the coverage report from codecov above, it looks like the lines where you raise ValueError for non-existent keys currently don't have any test coverage. Please add unit tests that verify the right error is raised, i.e., with pytest.raises.

max-sixty · 2019-07-02T06:07:56Z

The code looks great now, nice work @jukent - very succinct

If we can add that small test that @shoyer references (can do it in one of the existing test, just two lines), and potentially there's a duplication in the whatsnew.

(I'm on vacation so don't wait for me to merge)

jukent · 2019-07-02T17:30:59Z

Added those tests for for the raised Value Error.

shoyer · 2019-07-02T18:22:11Z

pep8speaks has a few minor style complaints to fix: #3045 (comment)

shoyer · 2019-07-02T20:23:13Z

xarray/tests/test_dataset.py

+        actual_2 = original.rename_vars(**name_dict)
+        assert_identical(expected, actual_2)
+
+        # Test to raise ValueError


For future reference, try to leave out redundant comments like this that literally describe how code works. If it would be just as clear to read the code directly, it's best to let code stand on its own.

shoyer · 2019-07-02T20:25:02Z

thanks @jukent !

max-sixty · 2019-07-03T09:53:54Z

Thanks @jukent! We're happy to have you as a contributor!

jukent added 8 commits June 25, 2019 11:50

Added rename_coords and rename_dims

65e3769

Removed white space from blank lines

4eaeaf5

Changed rename_coords to rename_vars

69c074e

Changed rename_coords to rename_vars in "See Also" or rename fx

7cd26ce

Fixed renaming dimension indexing

bc6d90b

Added testing for rename_vars and rename_dims

babf193

Merge branch 'master' of github.com:jukent/xarray

e74330b

Testing and fx for renaming vars and dims

588f9c8

Met pep8 standards

7505883

max-sixty requested changes Jun 25, 2019

View reviewed changes

xarray/tests/test_dataset.py Outdated Show resolved Hide resolved

Undid autopep8 for lines w noqa

9215908

max-sixty reviewed Jun 26, 2019

View reviewed changes

xarray/tests/test_dataset.py Outdated Show resolved Hide resolved

xarray.code-workspace Outdated Show resolved Hide resolved

Update xarray/tests/test_dataset.py

8753345

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

max-sixty requested changes Jun 26, 2019

View reviewed changes

xarray/core/dataset.py Show resolved Hide resolved

xarray/core/dataset.py Outdated Show resolved Hide resolved

xarray/tests/test_dataset.py Show resolved Hide resolved

xarray/tests/test_dataset.py Show resolved Hide resolved

xarray/tests/test_dataset.py Show resolved Hide resolved

jukent added 2 commits June 26, 2019 11:27

Cleaned up helper fxs and added actual_2 test

b790160

Merge branch 'master' of github.com:jukent/xarray

50242b3

max-sixty reviewed Jun 26, 2019

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

max-sixty reviewed Jun 26, 2019

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

jukent and others added 3 commits June 26, 2019 11:43

Update xarray/core/dataset.py

ae0deb1

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

Update xarray/core/dataset.py

cf28dde

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

deleted misc file

a73390d

max-sixty reviewed Jun 26, 2019

View reviewed changes

xarray/core/dataset.py Outdated Show resolved Hide resolved

jukent and others added 3 commits June 26, 2019 11:59

Update xarray/core/dataset.py

a0d4ca1

Co-Authored-By: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>

fixed conflicts

6ffdc4c

Merge branch 'master' of github.com:jukent/xarray

1494237

_rename_var_dims_helper undefined test

bdfd181

Use separate rename_dims and rename_vars dictionaries

a77820c

jukent and others added 5 commits June 27, 2019 12:32

Fixed documentation and added inplace back

c19ddb3

removing changes from rename

ef5cc66

removed test set up to fail (will add back)

1a08c51

fixed coord vs variable in test rename_

97c4e85

Merge branch 'master' into master

4e2bc76

jukent mentioned this pull request Jul 1, 2019

Rename dims independently from coords? #3026

Closed

dcherian reviewed Jul 1, 2019

View reviewed changes

doc/whats-new.rst Outdated Show resolved Hide resolved

Moved rename_var to under new fx/methods

caad102

Update whats-new.rst

09c801a

dcherian changed the title ~~Reopened rename_vars and rename_dims fxs with tests!~~ Add rename_vars and rename_dims Jul 1, 2019

use pytest.raises to test for ValueError

f4e1479

did not assign failed

c97ff0d

pep8 compliance

b3232ad

shoyer reviewed Jul 2, 2019

View reviewed changes

shoyer merged commit af37ddd into pydata:master Jul 2, 2019

keewis mentioned this pull request Sep 21, 2019

fix the name of the return value in the swap_dims docstrings #3329

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rename_vars and rename_dims #3045

Add rename_vars and rename_dims #3045

jukent commented Jun 25, 2019 •

edited

Loading

jukent commented Jun 25, 2019

pep8speaks commented Jun 25, 2019 •

edited

Loading

max-sixty commented Jun 25, 2019

max-sixty left a comment

dcherian commented Jun 26, 2019

max-sixty commented Jun 26, 2019

jukent commented Jun 26, 2019

shoyer commented Jun 26, 2019

jukent commented Jun 26, 2019

jukent commented Jul 1, 2019

dcherian left a comment

codecov bot commented Jul 1, 2019 •

edited

Loading

dcherian commented Jul 1, 2019

shoyer commented Jul 2, 2019

max-sixty commented Jul 2, 2019

jukent commented Jul 2, 2019

shoyer commented Jul 2, 2019

shoyer Jul 2, 2019 •

edited

Loading

shoyer commented Jul 2, 2019

max-sixty commented Jul 3, 2019

Add rename_vars and rename_dims #3045

Add rename_vars and rename_dims #3045

Conversation

jukent commented Jun 25, 2019 • edited Loading

jukent commented Jun 25, 2019

pep8speaks commented Jun 25, 2019 • edited Loading

Comment last updated at 2019-07-02 18:28:57 UTC

max-sixty commented Jun 25, 2019

max-sixty left a comment

Choose a reason for hiding this comment

dcherian commented Jun 26, 2019

max-sixty commented Jun 26, 2019

jukent commented Jun 26, 2019

shoyer commented Jun 26, 2019

jukent commented Jun 26, 2019

jukent commented Jul 1, 2019

dcherian left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 1, 2019 • edited Loading

Codecov Report

dcherian commented Jul 1, 2019

shoyer commented Jul 2, 2019

max-sixty commented Jul 2, 2019

jukent commented Jul 2, 2019

shoyer commented Jul 2, 2019

shoyer Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

shoyer commented Jul 2, 2019

max-sixty commented Jul 3, 2019

jukent commented Jun 25, 2019 •

edited

Loading

pep8speaks commented Jun 25, 2019 •

edited

Loading

codecov bot commented Jul 1, 2019 •

edited

Loading

shoyer Jul 2, 2019 •

edited

Loading