ENH: implement fill_value for df.add(other=Series) #13488 #32335
Conversation
| @@ -771,6 +793,12 @@ def f(self, other, axis=default_axis, level=None, fill_value=None): | |||
| if _should_reindex_frame_op(self, other, axis, default_axis, fill_value, level): | |||
| return _frame_arith_method_with_reindex(self, other, op) | |||
|
|
|||
| if not is_number(fill_value) and fill_value is not None: | |||
There was a problem hiding this comment.
i dont think this condition is right. what if the series we are working with is e.g. datetime64?
| @@ -10,6 +10,16 @@ including other versions of pandas. | |||
|
|
|||
| .. --------------------------------------------------------------------------- | |||
|
|
|||
| .. _whatsnew_102.enhancements: | |||
There was a problem hiding this comment.
this would go in the 1.1.0 file
|
All in all, I'd rather deprecate support for fill_value and ask users to do |
|
@jbrockmendel Thank you for reviewing this PR. I'd be happy to try a PR for the deprecation of support for fill_value. I'm new to open source (this is my first contribution) so I'd appreciate it if you could let me know whether this deprecation is worth spending my time on. Otherwise, I'll select another open Pandas issue to work on. |
|
@jonathanrhughes welcome! I would not advise doing that deprecation right away since it has not been agreed upon. Your best bet is to look at issues with the "good first issue" tag (skip the first page or two of results, as those have gotten crowded). |
I would agree with this
And this. Let's see what others think |
WillAyd
added
the
Needs Discussion
|
But in any case @jonathanrhughes thanks for pushing this PR. Very impressive for a first time contribution! |
|
@WillAyd I appreciate the feedback. Thank you. |
|
Well its been 10 days and I haven't seen anything in support of this so going to close. But in any case thanks again @jonathanrhughes - the PR is impressive just not something we have an appetite for |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff