Adding dict support for renaming columns#671
Adding dict support for renaming columns#671henrydingliu wants to merge 1 commit intocasact:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #671 +/- ##
==========================================
+ Coverage 85.03% 85.04% +0.01%
==========================================
Files 85 85
Lines 4890 4896 +6
Branches 627 629 +2
==========================================
+ Hits 4158 4164 +6
Misses 522 522
Partials 210 210
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@genedan bumping this to the top of your inbox |
|
Got it. I'll take a look at this tomorrow. |
|
|
||
|
|
||
| def test_rename_exception(genins, clrd) -> None: | ||
| # Test incorrect value argument - misspelling of string. |
There was a problem hiding this comment.
Not sure if I understood this comment. To me it seems to be testing 2 things at once, trying to map a new name to a non-existing column, and attempting to do a rename meant for columns, but accidentally applied to the origin instead, and then alerting the user by raising an error.
However, doing something similar in pandas doesn't actually cause an error. For example, if we create a DataFrame with columns A and B but pass a mapping {'C': 'bar'} whose key not only does not match either column but also gets accidentally passed to the index parameter, we just get a copy of the original DataFrame with no error.
import pandas as pd
df = pd.DataFrame({"A": [1, 2, 3], "B": [4, 5, 6]})
df
Out[4]:
A B
0 1 4
1 2 5
2 3 6
df.rename(index={'C': 'bar'})
Out[5]:
A B
0 1 4
1 2 5
2 3 6
I believe this was intentional by design for pandas, for example if we want to apply a mapping where a DataFrame only hit a subset of the keys (or even none), we'd still want it to work.
Eventually, to match the functionality of DataFrame.rename, we'll need to remove the value parameter and supply the mapping to other parameters, working our way towards matching the signature of DataFrame.rename:
DataFrame.rename(mapper=None, *, index=None, columns=None, axis=None, copy=<no_default>, inplace=False, level=None, errors='ignore')
But I remember we talked about moving these further changes to other issues. This PR still makes sense given the chainladder's current behavior, so I can approve this as long as we're aligned that the function's signature will change to match DataFrame.rename (or get as close as possible) in future PRs. Alternatively, if you'd like to tackle #662 in this PR, or even go all the way to get the signatures to match in this PR, I can review that too.
There was a problem hiding this comment.
i'm not following. this test was named test_rename_exception. since i added a new exception to the rename function, I added a clause to test the new exception.
what would you like to see changed?
Addressing #660