Skip to content

Conversation

thoo
Copy link
Contributor

@thoo thoo commented Feb 4, 2019

@pep8speaks
Copy link

pep8speaks commented Feb 4, 2019

Hello @thoo! Thanks for updating the PR.

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

Comment last updated on February 28, 2019 at 01:28 Hours UTC

@codecov
Copy link

codecov bot commented Feb 4, 2019

Codecov Report

Merging #25132 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #25132   +/-   ##
=======================================
  Coverage   92.37%   92.37%           
=======================================
  Files         166      166           
  Lines       52408    52408           
=======================================
  Hits        48411    48411           
  Misses       3997     3997
Flag Coverage Δ
#multiple 90.79% <ø> (ø) ⬆️
#single 42.86% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/tools/numeric.py 100% <ø> (ø) ⬆️
pandas/core/arrays/datetimelike.py 97.67% <ø> (ø) ⬆️
pandas/core/indexes/multi.py 95.6% <ø> (ø) ⬆️
pandas/core/series.py 93.68% <ø> (ø) ⬆️
pandas/core/panel.py 97.91% <ø> (ø) ⬆️
pandas/tseries/frequencies.py 97.7% <ø> (ø) ⬆️
pandas/core/frame.py 96.81% <ø> (ø) ⬆️
pandas/io/formats/style.py 96.69% <ø> (ø) ⬆️
pandas/core/generic.py 96.63% <ø> (ø) ⬆️
pandas/plotting/_core.py 83.58% <ø> (ø) ⬆️
... and 6 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 d3c9d6e...2c4455c. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 4, 2019

Codecov Report

Merging #25132 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25132      +/-   ##
==========================================
- Coverage   91.74%   91.74%   -0.01%     
==========================================
  Files         173      173              
  Lines       52923    52923              
==========================================
- Hits        48555    48554       -1     
- Misses       4368     4369       +1
Flag Coverage Δ
#multiple 90.31% <100%> (ø) ⬆️
#single 41.73% <100%> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/datetimelike.py 97.67% <ø> (ø) ⬆️
pandas/core/indexes/multi.py 95.62% <ø> (ø) ⬆️
pandas/core/series.py 93.68% <ø> (ø) ⬆️
pandas/tseries/frequencies.py 97.7% <ø> (ø) ⬆️
pandas/core/frame.py 96.85% <ø> (ø) ⬆️
pandas/core/generic.py 94.16% <ø> (ø) ⬆️
pandas/core/algorithms.py 94.77% <ø> (ø) ⬆️
pandas/plotting/_misc.py 38.68% <ø> (ø) ⬆️
pandas/core/arrays/categorical.py 95.97% <ø> (ø) ⬆️
pandas/io/excel/_base.py 92.48% <ø> (ø) ⬆️
... and 3 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 c986386...2d62018. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented Feb 4, 2019

can you merge master. RT04 is now active, so I think you need to adjust some things.

@jreback jreback added the Docs label Feb 4, 2019
thoo added 3 commits February 4, 2019 10:31
* upstream/master:
  TST/REF: collect DataFrame reduction tests (#24914)
  DOC: Improve docstring of Series.mul (#25136)
  Removal of return variable names (#25123)
  BUG: Fixing regression in DataFrame.all and DataFrame.any with bool_only=True (#25102)
  Reading a HDF5 created in py2 (#25058)
  DOC: Fix validation type error RT04 (#25107) (#25129)
thoo added 3 commits February 4, 2019 19:44
* upstream/master:
  Fix validation error type `SS05` and check in CI  (#25133)
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Looks good, some special cases can probably be improved

thoo added 7 commits February 5, 2019 20:05
* upstream/master:
  DOC: Fix validation type error SA05 (#25208)
  REF: Add more pytest idiom to test_holiday.py (#25204)
  DOC/CLN: Fix errors in Series docstrings (#24945)
  TST: follow-up to Test nested pandas array #24993 (#25155)
  modernize compat imports (#25192)
  fix MacPython pandas-wheels failure (#25186)
  BUG: DataFrame.merge(suffixes=) does not respect None (#24819)
  DEPR: remove PanelGroupBy, disable DataFrame.to_panel (#25047)
  DOC: update docstring for series.nunique (#25116)
  CLN: Use ABCs in set_index (#25128)
  BLD: pin cython language level to '2' (#25145)
  DOC: Updates to Timestamp document (#25163)
  STY: use pytest.raises context manager (indexes/multi) (#25175)
  Fixed tuple to List Conversion in Dataframe class (#25089)
@jreback jreback added this to the 0.25.0 milestone Feb 8, 2019
thoo added 2 commits February 8, 2019 15:00
* upstream/master:
  fix ci failures (#25225)
  STY: use pytest.raises context manager (indexes/period) (#25199)
  DOC: exclude autogenerated c/cpp/html files from 'trailing whitespace' checks (#24549)
  DOC: Fixes to docstrings and add PR10 (space before colon) to validation (#25109)
  REF: Remove many Panel tests (#25191)
  BUG: Fix Series.is_unique with single occurrence of NaN (#25182)
@jreback
Copy link
Contributor

jreback commented Feb 9, 2019

@datapythonista if you'd re-look. merge when good.

* upstream/master:
  BUG: Fix exceptions when Series.interpolate's `order` parameter is missing or invalid (#25246)
  API: Ensure DatetimeTZDtype standardizes pytz timezones (#25254)
  Split Excel IO Into Sub-Directory (#25153)
  PR04 errors fix (#25157)
  DEPR: remove assert_panel_equal (#25238)
  BUG: pandas Timestamp tz_localize and tz_convert do not preserve `freq` attribute (#25247)
  Revert "BLD: prevent asv from calling sys.stdin.close() by using different launch method (#25237)" (#25253)
  REF/TST: resample/test_base.py (#25262)
  BUG: Duplicated returns boolean dataframe (#25234)
  CLN: Remove ipython 2.x compat (#25150)
  Refactor groupby group_add from tempita to fused types (#24954)
  CLN: For loops, boolean conditions, misc. (#25206)
  (Closes #25029) Removed extra bracket from cheatsheet code example. (#25032)
  BLD: prevent asv from calling sys.stdin.close() by using different launch method (#25237)
  BUG: Fix read_json orient='table' without index (#25170) (#25171)
  BUG: Fix regression in DataFrame.apply causing RecursionError (#25230)
  BUG-25061 fix printing indices with NaNs (#25202)
  DEPR: Add Deprecated warning for timedelta with passed units M and Y  (#23264)
  DEPR: Remove Panel-specific parts of io.pytables (#25233)
  DEPR: remove tm.makePanel and all usages (#25231)
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Great job, thanks for all the fixes. Just few comments.

If the input is an Index, the return is an Index
If the input is a Categorical dtype, the return is a Categorical
If the input is a Series/ndarray, the return will be an ndarray
unique values
Copy link
Member

Choose a reason for hiding this comment

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

This should be the type, can you find out what's the returned Python type, and move this to the description please?

Returns
-------
values : numpy array
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
values : numpy array
numpy.array

Returns
-------
values : numpy array
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
values : numpy array
numpy.array

Returns
-------
value : scalar value
scalar value
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
scalar value
scalar

Returns
-------
sanitized_column : numpy-array
numpy-array
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
numpy-array
numpy.array

Returns
-------
axes : matplotlib.AxesSubplot histogram.
matplotlib.AxesSubplot histogram.
Copy link
Member

Choose a reason for hiding this comment

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

Leave just the type, say it's a Histogram in the description.

Returns
-------
ax : Matplotlib axis object
Matplotlib axis object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Matplotlib axis object
class:`matplotlib.axes.Axes`

There are many like this afterwards

Returns
-------
ax: matplotlib axis object
matplotlib axis object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
matplotlib axis object
class:`matplotlib.axes.Axes`

Returns
-------
ax: Matplotlib axis object
Matplotlib axis object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Matplotlib axis object
class:`matplotlib.axes.Axes`

Returns:
-----------
ax: Matplotlib axis object
Matplotlib axis object
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Matplotlib axis object
class:`matplotlib.axes.Axes`

@thoo
Copy link
Contributor Author

thoo commented Feb 14, 2019

@datapythonista I think #25108 is already closed by #25309. Should I fix with your suggestions which are not covered by #25309? Or just close this PR and submit the new one?

@WillAyd
Copy link
Member

WillAyd commented Feb 14, 2019

@thoo sorry didn't realize we had two PRs for this and other is already merged for RT05. Are there other elements to this outside of that you are trying to merge?

@jreback
Copy link
Contributor

jreback commented Feb 17, 2019

pls merge master again

@jreback
Copy link
Contributor

jreback commented Feb 19, 2019

cc @datapythonista

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @thoo


MSG='Validate docstrings (GL06, GL07, GL09, SS04, PR03, PR05, PR10, EX04, RT04, RT05, SS05, SA05)' ; echo $MSG
$BASE_DIR/scripts/validate_docstrings.py --format=azure --errors=GL06,GL07,GL09,SS04,PR03,PR04,PR05,EX04,RT04,RT05,SS05,SA05
MSG='Validate docstrings (GL06, GL07, GL09, SS04, SS05, PR03, PR04, PR05, EX04, RT04, RT05, SA05)' ; echo $MSG
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason for removing PR10 here or just mistake?

Copy link
Contributor Author

@thoo thoo Feb 20, 2019

Choose a reason for hiding this comment

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

It is just a mistake from merge.

Copy link
Contributor Author

@thoo thoo Feb 20, 2019

Choose a reason for hiding this comment

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

PR10 is actually not included in the current check at line 245. So I wasn't sure someone exclude PR10 but forgot to remove from the message.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I think this was just missed in #25109 so makes sense to update here accordingly

Copy link
Contributor Author

@thoo thoo Feb 21, 2019

Choose a reason for hiding this comment

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

##[error]pandas/io/excel/_base.py(492,): error PR10: pandas.ExcelWriter: Parameter ".. versionadded" requires a space before the colon separating the parameter name and type

PR10 is not passing the check with .. versionadded:: 0.24.. I am going to leave PR10 for now. Update pandas.ExcelWriter

* Interval
* Sparse
* IntegerNA
* IntegerNA .
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this on quite a few other requests. Nuanced but I'm personally of the opinion that this is a "hack" to pass validation so would prefer another route. Can you just add a one-line description after the bullet points instead?

thoo added 3 commits February 20, 2019 14:21
* upstream/master:
  API: more consistent error message for MultiIndex.from_arrays (#25189)
  fix typo of see also in DataFrame stat funcs (#25388)
  edited whatsnew typo (#25381)
  REGR: fix TimedeltaIndex sum and datetime subtraction with NaT (#25282, #25317) (#25329)
  ENH: indexing and __getitem__ of dataframe and series accept zerodim integer np.array as int (#24924)
  [CLN] Excel Module Cleanups (#25275)
  Interval dtype fix (#25338)
  BUG/ENH: Timestamp.strptime (#25124)
  14873: test for groupby.agg coercing booleans (#25327)
  [BUG] exception handling of MultiIndex.__contains__ too narrow (#25268)
  9236: test for the DataFrame.groupby with MultiIndex having pd.NaT (#25310)
  #23049: test for Fatal Stack Overflow stemming From Misuse of astype('category') (#25366)
  Remove spurious MultiIndex creation in `_set_axis_name` (#25371)
  DOC: modify typos in Contributing section (#25365)
  DOC/BLD: fix --no-api option (#25209)
  DOC: Correct doc mistake in combiner func (#25360)
@WillAyd
Copy link
Member

WillAyd commented Feb 21, 2019

@thoo looking pretty good thanks for addressing comments. Can you update title to reflect what this PR is actually addressing, since it's no longer RT05?

@thoo thoo changed the title Fix validation error type RT05 and check in CI ~Fix validation error type RT05 and check in CI~ Clean up docstrings from functions related to RT05 errors Feb 21, 2019
@thoo thoo changed the title ~Fix validation error type RT05 and check in CI~ Clean up docstrings from functions related to RT05 errors Clean up docstrings from functions related to RT05 errors Feb 21, 2019
@thoo thoo changed the title Clean up docstrings from functions related to RT05 errors Fix PR10 error and Clean up docstrings from functions related to RT05 errors Feb 21, 2019
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Minor things but otherwise lgtm and thanks for updating title.

@datapythonista - some changes since your last approval do you mind taking a look again?

- If DataFrame.agg is called with several functions, returns
a DataFrame.
- If Series.agg is called with single function, returns a scalar.
- If Series.agg is called with several functions, returns a Series.
Copy link
Member

Choose a reason for hiding this comment

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

Let's do the same thing you are doing below, where instead of putting periods at the end of each bullet you end with a short sentence after the bullets

- If the input is an Index, the return is an Index.
- If the input is a Categorical dtype, the return is a Categorical.
- If the input is a Series/ndarray, the return will be an ndarray.
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here around periods for bullet points

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

looks good, jut couple of comments more besides the ones already mentioned

indexer, keyarr
indexer is an ndarray or None if cannot convert
keyarr are tuple-safe keys
keyarr are tuple-safe keys.
Copy link
Member

Choose a reason for hiding this comment

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

do you mind documenting both parameters separately? It's done in other parts of this PR

File mode to use (write or append).
.. versionadded:: 0.24.0
File mode to use (write or append). .. versionadded:: 0.24.0
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason for this? Not sure if this will render correctly in the html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's right. I was about to fix it.

thoo added 2 commits February 21, 2019 23:38
* upstream/master:
  CLN: (re-)enable infer_dtype to catch complex (#25382)
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

All good, just the Returns with the creative documenting of multiple values needs to be fixed

- indexer : an ndarray or None if cannot convert
- keyarr : tuple-safe keys
Return indexer, keyarr.
Copy link
Member

Choose a reason for hiding this comment

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

Can you check in this diff the previous change? That's how a Return with multiple values is documented.

@WillAyd
Copy link
Member

WillAyd commented Feb 27, 2019

@thoo can you merge master and ping on green?

* upstream/master:
  DOC: CategoricalIndex doc string (#24852)
  CI: add __init__.py to isort skip list (#25455)
  TST: numpy RuntimeWarning with Series.round() (#25432)
  DOC: fixed geo accessor example in extending.rst (#25420)
  BUG: fixed merging with empty frame containing an Int64 column (#25183) (#25289)
  TST: remove never-used singleton fixtures (#24885)
  PERF/REF: improve performance of Series.searchsorted, PandasArray.searchsorted, collect functionality (#22034)
  BUG: Indexing with UTC offset string no longer ignored (#25263)
  API/ERR: allow iterators in df.set_index & improve errors (#24984)
  DOC: Rewriting of ParserError doc + minor spacing (#25421)
  ENH: Add in sort keyword to DatetimeIndex.union (#25110)
  ERR: doc update for ParsingError (#25414)
  BUG: Fix type coercion in read_json orient='table' (#21345) (#25219)
  DEP: add pytest-mock to environment.yml (#25417)
  Correct a typo of version number for interpolate() (#25418)
  Mark test_pct_max_many_rows as high memory (#25400)
  DOC: Edited docstring of Interval (#25410)
@thoo
Copy link
Contributor Author

thoo commented Mar 1, 2019

@WillAyd All checks passed.

@WillAyd WillAyd merged commit 0a61ecd into pandas-dev:master Mar 1, 2019
@WillAyd
Copy link
Member

WillAyd commented Mar 1, 2019

Thanks @thoo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix validation error type RT05 and check in CI

5 participants