DOC: Clarify and add fill_value example in arithmetic ops #19675

HagaiHargil · 2018-02-13T14:32:36Z

closes DOC: Clarifiy fill_value behavior in arithmetic ops #19653
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

pep8speaks · 2018-02-13T14:32:39Z

Hello @HagaiHargil! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 21, 2018 at 06:55 Hours UTC

TomAugspurger · 2018-02-13T15:26:54Z

pandas/core/ops.py

- missing, the result will be missing
+ Fill existing missing (NaN) values, and any new element needed for 
+ successful array alignment, with this value before computation. 
+ If data in both corresponding DataFrame locations is missing 


This is for Series, so change DataFrame to Series.

I would maybe rephrase it as

Fill missing values with 'fill_value'. There are two sources of missing values * Missing values present either Series before the operation * Newly created missing values as a result of an alignment In either case, missing values are not filled if both Series are missing after alignment. At least one value from either Series must be not NA for 'fill_value' to be used.

Though I'm sure if this is any clearer.

I have a problem with the first line - "Fill missing values with 'fill_value'". This is basically what bothered me in the first place, it doesn't exactly fill missing value, in the naive way you would expect it to.

If you insist I'll gladly rephrase my current wording. In the mean time I'll fix the rest of your remarks.

TomAugspurger · 2018-02-13T16:06:55Z

pandas/core/ops.py

+
+Examples
+--------
+>>> a = pd.Series([1, 1, 1, np.nan], index=['a', 'b', 'c', 'd'])


Can you use a DataFrame for this example? It can just be a single-column DataFrame with these same values and index.

jreback

if you want to make a common shared doc-string would be nice as well (could be followon PR)

jreback · 2018-02-15T12:21:21Z

pandas/core/ops.py

 fill_value : None or float value, default None (NaN)
- Fill missing (NaN) values with this value. If both Series are
- missing, the result will be missing
+ Fill existing missing (NaN) values, and any new element needed for


I would remove the 'and any new elemnt needed for successful array alignment', this is redundant with your last sentence.

I'm not sure which sentence are you referring to. The "and any new element needed" sentence refers to the alignment process. The last sentence ("If data in both corresponding...") only deals with a "corner case" of two NaNs, without any direct reference to the data alignment process.

don't use the word 'array' as these are not arrays. this is still not very clear.

I like this phrasing much better. Could you change "new element" to "new missing values"

You can remove the commas around the "and any new element... " clause.

jreback · 2018-02-15T12:21:40Z

pandas/core/ops.py


+Examples
+--------
+>>> a = pd.Series([1, 1, 1, np.nan], index=['a', 'b', 'c', 'd'])


show a and b as well

You need to have a line with just >>> a on it before showing the Series. Likewise for b.

jreback · 2018-02-15T12:21:49Z

pandas/core/ops.py

 fill_value : None or float value, default None
- Fill missing (NaN) values with this value. If both DataFrame locations are
- missing, the result will be missing
+ Fill existing missing (NaN) values, and any new element needed for


jreback · 2018-02-15T12:22:06Z

pandas/core/ops.py

+
+Examples
+--------
+>>> a = pd.DataFrame([1, 1, 1, np.nan], index=['a', 'b', 'c', 'd'])


show a and b.

prob want to show say a with 1 column and b with 2

jreback · 2018-02-15T12:22:18Z

pandas/core/ops.py


+Examples
+--------
+>>> a = pd.DataFrame([1, 1, 1, np.nan], index=['a', 'b', 'c', 'd'])


jreback · 2018-02-15T12:23:10Z

pandas/core/ops.py

+
+Examples
+--------
+>>> a = pd.DataFrame([1, 1, 1, np.nan], index=['a', 'b', 'c', 'd'])


prob want to show say a with 1 column and b with 2

codecov · 2018-02-18T07:24:37Z

Codecov Report

Merging #19675 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@ Coverage Diff @@ ## master #19675 +/- ## ========================================== + Coverage 91.57% 91.61% +0.03%  ========================================== Files 150 150 Lines 48817 48882 +65 ========================================== + Hits 44704 44782 +78  + Misses 4113 4100 -13

Flag	Coverage Δ
#multiple	`89.98% <100%> (+0.03%)`	⬆️
#single	`41.79% <100%> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/ops.py	`96.86% <100%> (+0.03%)`	⬆️
pandas/core/arrays/base.py	`60% <0%> (-0.61%)`	⬇️
pandas/core/series.py	`94.46% <0%> (-0.11%)`	⬇️
pandas/core/sparse/series.py	`95.25% <0%> (-0.02%)`	⬇️
pandas/core/indexes/multi.py	`95.06% <0%> (-0.01%)`	⬇️
pandas/core/sparse/frame.py	`94.81% <0%> (ø)`	⬆️
pandas/core/groupby.py	`92.2% <0%> (ø)`	⬆️
pandas/core/indexes/api.py	`98.78% <0%> (ø)`	⬆️
pandas/io/parquet.py	`71.79% <0%> (ø)`	⬆️
... and 14 more

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 d9551c8...3192301. Read the comment docs.

HagaiHargil · 2018-02-18T07:26:31Z

I'm also not sure what a shared docstring really is in this context :)

jreback · 2018-02-18T17:30:23Z

pandas/core/ops.py

+b 1.0
+c 1.0
+d NaN
+>>> b = pd.DataFrame([[1, np.nan], [np.nan, 2], [1, np.nan], [np.nan, 2]], index=['a', 'b', 'c_', 'd'])


this will not pass the linter, need to format this, prob easier to use a dict to construct as well.

jreback · 2018-02-18T17:31:08Z

pandas/core/ops.py

 fill_value : None or float value, default None (NaN)
- Fill missing (NaN) values with this value. If both Series are
- missing, the result will be missing
+ Fill existing missing (NaN) values, and any new element needed for


don't use the word 'array' as these are not arrays. this is still not very clear.

HagaiHargil · 2018-02-18T17:56:54Z

I changed the confusing "array" into series\dataframe.

Regarding the "this is still not very clear" comment -

First, I guess you agree with me that the previous text was plain wrong. Now I'm trying to find a clear enough sentence that fits the actual behavior of this keyword, but inconsistent behavior leads to badly-phrased docs. I'm really not sure what to say anymore.

TomAugspurger · 2018-02-20T13:11:29Z

Looks like there are some linting errors failing the build: #19675 (comment)

This also seems to cause issues with our _make_flex_doc helper. Are you able to import pandas after making your changes locally?

HagaiHargil · 2018-02-20T18:09:38Z

It was an interesting bug with the {} dict constructor.

jreback · 2018-02-21T00:14:25Z

pandas/core/ops.py

+c 1.0
+d NaN
+dtype: float64
+>>> b = pd.Series([1, np.nan, 1, np.nan], index=['a', 'b', 'c_', 'd'])


i would prefer just using abde or something, the c_ is confusing

jreback · 2018-02-22T00:16:25Z

thanks @HagaiHargil

…#19675)

HagaiHargil added 3 commits February 13, 2018 14:41

Clarified fill_value parameter in arithmetic ops

24d704a

Add fill_value change to DF

b5d9d6f

Add fill_value example to DF

c45dbd9

HagaiHargil mentioned this pull request Feb 13, 2018

DOC: Clarifiy fill_value behavior in arithmetic ops #19653

Closed

TomAugspurger reviewed Feb 13, 2018

View reviewed changes

Rephrasing and PEP8 fixes

b2794d9

gfyoung added Docs Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Numeric Operations Arithmetic, Comparison, and Logical operations labels Feb 14, 2018

jreback requested changes Feb 15, 2018

View reviewed changes

jreback changed the title ~~Clarify and add fill_value example in arithmetic ops~~ DOC: Clarify and add fill_value example in arithmetic ops Feb 15, 2018

Two-columned DataFrame

13f1f00

jreback requested changes Feb 18, 2018

View reviewed changes

dict constructor and rephrasing

1bf321c

Variables are now displayed before operation

fd89953

Fixed bug with format and dict constructor

af0aebe

jreback requested changes Feb 21, 2018

View reviewed changes

Changes index from c_ to e

3192301

jreback added this to the 0.23.0 milestone Feb 22, 2018

jreback approved these changes Feb 22, 2018

View reviewed changes

jreback merged commit 4ed8313 into pandas-dev:master Feb 22, 2018

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

DOC: Clarify and add fill_value example in arithmetic ops (pandas-dev…

4ea1508

…#19675)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

DOC: Clarify and add fill_value example in arithmetic ops #19675

DOC: Clarify and add fill_value example in arithmetic ops #19675

Uh oh!

HagaiHargil commented Feb 13, 2018

pep8speaks commented Feb 13, 2018 •

edited

Loading

TomAugspurger Feb 13, 2018

TomAugspurger Feb 13, 2018

HagaiHargil Feb 13, 2018

TomAugspurger Feb 13, 2018

jreback left a comment

jreback Feb 15, 2018

HagaiHargil Feb 18, 2018

jreback Feb 18, 2018

TomAugspurger Feb 18, 2018

jreback Feb 15, 2018

TomAugspurger Feb 18, 2018

jreback Feb 15, 2018

jreback Feb 15, 2018

jreback Feb 15, 2018

jreback Feb 15, 2018

jreback Feb 15, 2018

codecov bot commented Feb 18, 2018 •

edited

Loading

HagaiHargil commented Feb 18, 2018

jreback Feb 18, 2018

jreback Feb 18, 2018

HagaiHargil commented Feb 18, 2018

TomAugspurger commented Feb 20, 2018

HagaiHargil commented Feb 20, 2018

jreback Feb 21, 2018

jreback commented Feb 22, 2018

Labels

5 participants

Uh oh!

DOC: Clarify and add fill_value example in arithmetic ops #19675

DOC: Clarify and add fill_value example in arithmetic ops #19675

Uh oh!

Conversation

HagaiHargil commented Feb 13, 2018

pep8speaks commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on February 21, 2018 at 06:55 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

HagaiHargil commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HagaiHargil commented Feb 18, 2018

TomAugspurger commented Feb 20, 2018

HagaiHargil commented Feb 20, 2018

Choose a reason for hiding this comment

jreback commented Feb 22, 2018

Labels

5 participants

pep8speaks commented Feb 13, 2018 •

edited

Loading

codecov bot commented Feb 18, 2018 •

edited

Loading