Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CLN: parametrize test_nat_comparisons #37195

Merged
merged 4 commits into from
Nov 4, 2020
Merged

CLN: parametrize test_nat_comparisons #37195

merged 4 commits into from
Nov 4, 2020

Conversation

ivanovmg
Copy link
Member

@ivanovmg ivanovmg commented Oct 17, 2020 •
edited
Loading

Parametrize TestDatetime64SeriesComparison.test_nat_comparisons
using operator module.

If this kind of cleanup is considered reasonable,
then I will go through the remaining tests in this test module.

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@ivanovmg
CLN: parametrize test_nat_comparisons
9906731 
Parametrize TestDatetime64SeriesComparison.test_nat_comparisons
using ``operator`` module.
@ivanovmg ivanovmg requested a review from jbrockmendel October 17, 2020 15:43
@jreback jreback added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Testing pandas testing functions or related to the test suite labels Oct 17, 2020
@jreback jreback added this to the 1.2 milestone Oct 17, 2020
@jreback
Copy link
Contributor

jreback commented Oct 17, 2020

yes this type of cleanup is good. targeted PRs are the best.

cc @jbrockmendel if comments.

@jbrockmendel
Copy link
Member

i do not find this clearer than the status quo.

the lowest-hanging fruit in the tests/arithmetic directory are ones that do not use the box_with_array fixture

there are also very likely redundant tests that can be de-duplicated

@jreback jreback removed this from the 1.2 milestone Oct 20, 2020
@jreback
Copy link
Contributor

jreback commented Oct 22, 2020

@jbrockmendel ok so you are -1 on this?

@jbrockmendel
Copy link
Member

-0.5, i.e. i dont think this is an improvement, but not a hill to die on if others have strong opinions

@jreback jreback added this to the 1.2 milestone Oct 23, 2020
@jreback
Copy link
Contributor

jreback commented Nov 4, 2020

ok closing this

@jreback jreback closed this Nov 4, 2020
@simonjayhawkins
Copy link
Member

-0.5, i.e. i dont think this is an improvement, but not a hill to die on if others have strong opinions

Parameterisation is beneficial when there a multiple test failures. It is sometimes easier to debug when all the passing and failing test cases are visible together.

This PR looked good to me.

@jreback jreback reopened this Nov 4, 2020
@jreback
Copy link
Contributor

jreback commented Nov 4, 2020

ok @ivanovmg can u merge master

@ivanovmg
Merge branch 'master' into cleanup/arithmetic
f5295e1
@ivanovmg
REF: rename operator variable to op
a8f78d8 
Rename local variable operator -> op.
This is done for consistency with other tests in the module.
And also not to confuse with ``operator`` module.
@ivanovmg
Copy link
Member Author

ivanovmg commented Nov 4, 2020

Merged master.
Also I renamed local variable operator into op for consistency with other test and not to confuse with module operator.
However, probably more readable option would be operator_ with a trailing underscore, so it is more clear that we are talking about the operator.
Alternatively, binary_operator - longer name, but more clear IMHO.

@ivanovmg ivanovmg requested a review from jreback November 4, 2020 14:43
@jbrockmendel
Copy link
Member

Parameterisation is beneficial when there a multiple test failures. It is sometimes easier to debug when all the passing and failing test cases are visible together.

This is true in a general sense, but I'd point out:

  1. we have many tests where we don't parametrize these particular 6 methods

  2. The 6 Series objects get created at test collection time, which increases the memory footprint of the test suite. In isolation these 6 small Series is not a big deal, but we are already at a point where memory is causing the py38 windows build to fail regularly.

Again, not a hill worth dying on, just saying.

jbrockmendel
jbrockmendel reviewed Nov 4, 2020
View reviewed changes
pandas/tests/arithmetic/test_datetime64.py Outdated
@@ -152,8 +152,6 @@ def test_dt64arr_nat_comparison(self, tz_naive_fixture, box_with_array):


class TestDatetime64SeriesComparison:
# TODO: moved from tests.series.test_operators; needs cleanup
Copy link
Member

Choose a reason for hiding this comment

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

this comment pertains to the entire class, not just this test

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Moved it back.

@jreback jreback merged commit 8c170c4 into pandas-dev:master Nov 4, 2020
@jreback
Copy link
Contributor

jreback commented Nov 4, 2020

thanks @ivanovmg

@ivanovmg ivanovmg deleted the cleanup/arithmetic branch November 6, 2020 15:36
jreback added a commit that referenced this pull request Nov 13, 2020
@Mikhaylov-yv @jbrockmendel
@MarcoGorelli @PCerles @jorisvandenbossche @ssche @MicaelJarniac @arw2019 @ivanovmg @erfannariman @fangchenli @phofl @attack68 @twoertwein @jreback @Japanuspus @abbywh @taytzehao @ma3da @junjunjunk @AlexKirko @ukarroum @charlesdong1991 @rhshadrach @simonjayhawkins
TST: split up tests/plotting/test_frame.py into subdir & modules #34769…
096f6be 
… (#37655)

* Moving the file test_frame.py to a new directory

* Сreated file test_frame_color.py

* Transfer tests
of test_frame.py
to test_frame_color.py

* PEP 8 fixes

* Transfer tests

of test_frame.py
to test_frame_groupby.py and test_frame_subplots.py

* Removing unnecessary imports

* PEP 8 fixes

* Fixed class name

* Transfer tests

of test_frame.py
to test_frame_subplots.py

* Transfer tests

of test_frame.py
to test_frame_groupby.py, test_frame_subplots.py, test_frame_color.py

* Changed class names

* Removed unnecessary imports

* Removed import

* catch FutureWarnings (#37587)

* TST/REF: collect indexing tests by method (#37590)

* REF: prelims for single-path setitem_with_indexer (#37588)

* ENH: __repr__ for 2D DTA/TDA (#37164)

* CLN: de-duplicate _validate_where_value with _validate_setitem_value (#37595)

* TST/REF: collect tests by method (#37589)

* TST/REF: move remaining setitem tests from test_timeseries

* TST/REF: rehome test_timezones test

* move misplaced arithmetic test

* collect tests by method

* move misplaced file

* REF: Categorical.is_dtype_equal -> categories_match_up_to_permutation (#37545)

* CLN refactor non-core (#37580)

* refactor core/computation (#37585)

* TST/REF: share method tests between DataFrame and Series (#37596)

* BUG: Index.where casting ints to str (#37591)

* REF: IntervalArray comparisons (#37124)

* regression fix for merging DF with datetime index with empty DF (#36897)

* ERR: fix error message in Period for invalid frequency (#37602)

* CLN: remove rebox_native (#37608)

* TST/REF: tests.generic (#37618)

* TST: collect tests by method (#37617)

* TST/REF: collect test_timeseries tests by method

* misplaced DataFrame.values tst

* misplaced dataframe.values test

* collect test by method

* TST/REF: share tests across Series/DataFrame (#37616)

* Gh 36562 typeerror comparison not supported between float and str (#37096)

* docs: fix punctuation (#37612)

* REGR: pd.to_hdf(..., dropna=True) not dropping missing rows (#37564)

* parametrize set_axis tests (#37619)

* CLN: clean color selection in _matplotlib/style (#37203)

* DEPR: DataFrame/Series.slice_shift (#37601)

* REF: re-use validate_setitem_value in Categorical.fillna (#37597)

* PERF: release gil for ewma_time (#37389)

* BUG: Groupy dropped nan groups from result when grouping over single column (#36842)

* ENH: implement timeszones support for read_json(orient='table') and astype() from 'object' (#35973)

* REF/BUG/TYP: read_csv shouldn't close user-provided file handles (#36997)

* BUG/REF: read_csv shouldn't close user-provided file handles

* get_handle: typing, returns is_wrapped, use dataclass, and make sure that all created handlers are returned

* remove unused imports

* added IOHandleArgs.close

* added IOArgs.close

* mostly comments

* move memory_map from TextReader to CParserWrapper

* moved IOArgs and IOHandles

* more comments

Co-authored-by: Jeff Reback <jeff@reback.net>

* more typing checks to pre-commit (#37539)

* TST: 32bit dtype compat test_groupby_dropna (#37623)

* BUG: Metadata propagation for groupby iterator (#37461)

* BUG: read-only values in cython funcs (#37613)

* CLN refactor core/arrays (#37581)

* Fixed Metadata Propogation in DataFrame (#37381)

* TYP: add Shape alias to pandas._typing (#37128)

* DOC: Fix typo (#37630)

* CLN: parametrize test_nat_comparisons (#37195)

* dataframe dataclass docstring updated (#37632)

* refactor core/groupby (#37583)

* BUG: set index of DataFrame.apply(f) when f returns dict (#37544) (#37606)

* BUG: to_dict should return a native datetime object for NumPy backed dataframes (#37571)

* ENH: memory_map for compressed files (#37621)

* DOC: add example & prose of slicing with labels when index has duplicate labels  (#36814)

* DOC: add example & prose of slicing with labels when index has duplicate labels #36251

* DOC: proofread the sentence.

Co-authored-by: Jun Kudo <jun-lab@junnoMacBook-Pro.local>

* DOC: Fix typo (#37636)

"columns(s)" sounded odd, I believe it was supposed to be just "column(s)".

* CI: troubleshoot win py38 builds (#37652)

* TST/REF: collect indexing tests by method (#37638)

* TST/REF: collect tests for get_numeric_data (#37634)

* misplaced loc test

* TST/REF: collect get_numeric_data tests

* REF: de-duplicate _validate_insert_value with _validate_scalar (#37640)

* CI: catch windows py38 OSError (#37659)

* share test (#37679)

* TST: match matplotlib warning message (#37666)

* TST: match matplotlib warning message

* TST: match full message

* pd.Series.loc.__getitem__ promotes to float64 instead of raising KeyError (#37687)

* REF/TST: misplaced Categorical tests (#37678)

* REF/TST: collect indexing tests by method (#37677)

* CLN: only call _wrap_results one place in nanmedian (#37673)

* TYP: Index._concat (#37671)

* BUG: CategoricalIndex.equals casting non-categories to np.nan (#37667)

* CLN: _replace_single (#37683)

* REF: simplify _replace_single by noting regex kwarg is bool

* Annotate

* CLN: remove never-False convert kwarg

* TYP: make more internal funcs keyword-only (#37688)

* REF: make Series._replace_single a regular method (#37691)

* REF: simplify cycling through colors (#37664)

* REF: implement _wrap_reduction_result (#37660)

* BUG: preserve fold in Timestamp.replace (#37644)

* CLN: Clean indexing tests (#37689)

* TST: fix warning for pie chart (#37669)

* PERF: reverted change from commit 7d257c6 to solve issue #37081 (#37426)

* DataFrameGroupby.boxplot fails when subplots=False (#28102)

* ENH: Improve error reporting for wrong merge cols (#37547)

* Transfer tests
of test_frame.py
to test_frame_color.py

* PEP8

* Fixes for linter

* Сhange pd.DateFrame to DateFrame

* Move inconsistent namespace check to pre-commit, fixup more files (#37662)

* check for inconsistent namespace usage

* doc

* typos

* verbose regex

* use verbose flag

* use verbose flag

* match both directions

* add test

* don't import annotations from future

* update extra couple of cases

* 🚚 rename

* typing

* don't use subprocess

* don't type tests

* use pathlib

* REF: simplify NDFrame.replace, ObjectBlock.replace (#37704)

* REF: implement Categorical.encode_with_my_categories (#37650)

* REF: implement Categorical.encode_with_my_categories

* privatize

* BUG: unpickling modifies Block.ndim (#37657)

* REF: dont support dt64tz in nanmean (#37658)

* CLN: Simplify groupby head/tail tests (#37702)

* Bug in loc raised for numeric label even when label is in Index (#37675)

* REF: implement replace_regex, remove unreachable branch in ObjectBlock.replace (#37696)

* TYP: Check untyped defs (except vendored) (#37556)

* REF: remove ObjectBlock._replace_single (#37710)

* Transfer tests
of test_frame.py
to test_frame_color.py

* TST/REF: collect indexing tests by method (#37590)

* PEP8

* Сhange DateFrame to pd.DateFrame

* Сhange pd.DateFrame to DateFrame

* Removing imports

* Bug fixes

* Bug fixes

* Fix incorrect merge

* test_frame_color.py edit

* Transfer tests

of test_frame.py
to test_frame_color.py, test_frame_groupby.py and test_frame_subplots.py

* Removing unnecessary imports

* PEP8

* # Conflicts:
#	pandas/tests/plotting/frame/test_frame.py
#	pandas/tests/plotting/frame/test_frame_color.py
#	pandas/tests/plotting/frame/test_frame_subplots.py

* Moving the file test_frame.py to a new directory

* Transfer tests

of test_frame.py
to test_frame_color.py, test_frame_groupby.py and test_frame_subplots.py

* Removing unnecessary imports

* PEP8

* CLN: clean categorical indexes tests (#37721)

* Fix merge error

* PEP 8 fixes

* Fix merge error

* Removing unnecessary imports

* PEP 8 fixes

* Fixed class name

* Transfer tests

of test_frame.py
to test_frame_subplots.py

* Transfer tests

of test_frame.py
to test_frame_groupby.py, test_frame_subplots.py, test_frame_color.py

* Changed class names

* Removed unnecessary imports

* Removed import

* TST/REF: collect indexing tests by method (#37590)

* TST: match matplotlib warning message (#37666)

* TST: match matplotlib warning message

* TST: match full message

* TST: fix warning for pie chart (#37669)

* Transfer tests
of test_frame.py
to test_frame_color.py

* PEP8

* Fixes for linter

* Сhange pd.DateFrame to DateFrame

* Transfer tests
of test_frame.py
to test_frame_color.py

* PEP8

* Сhange DateFrame to pd.DateFrame

* Сhange pd.DateFrame to DateFrame

* Removing imports

* Bug fixes

* Bug fixes

* Fix incorrect merge

* test_frame_color.py edit

* Fix merge error

* Fix merge error

* Removing unnecessary features

* Resolving Commit Conflicts daf999f 365d843

* black fix

Co-authored-by: jbrockmendel <jbrockmendel@gmail.com>
Co-authored-by: Marco Gorelli <m.e.gorelli@gmail.com>
Co-authored-by: Philip Cerles <philip.cerles@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Sven <sven.schellenberg@paradynsystems.com>
Co-authored-by: Micael Jarniac <micael@jarniac.com>
Co-authored-by: Andrew Wieteska <48889395+arw2019@users.noreply.github.com>
Co-authored-by: Maxim Ivanov <41443370+ivanovmg@users.noreply.github.com>
Co-authored-by: Erfan Nariman <34067903+erfannariman@users.noreply.github.com>
Co-authored-by: Fangchen Li <fangchen.li@outlook.com>
Co-authored-by: patrick <61934744+phofl@users.noreply.github.com>
Co-authored-by: attack68 <24256554+attack68@users.noreply.github.com>
Co-authored-by: Torsten Wörtwein <twoertwein@users.noreply.github.com>
Co-authored-by: Jeff Reback <jeff@reback.net>
Co-authored-by: Janus <janus@insignificancegalore.net>
Co-authored-by: Joel Whittier <rootbeerfriend@gmail.com>
Co-authored-by: taytzehao <jtth95@gmail.com>
Co-authored-by: ma3da <34522496+ma3da@users.noreply.github.com>
Co-authored-by: junk <juntrp0207@gmail.com>
Co-authored-by: Jun Kudo <jun-lab@junnoMacBook-Pro.local>
Co-authored-by: Alex Kirko <alexander.kirko@gmail.com>
Co-authored-by: Yassir Karroum <ukarroum17@gmail.com>
Co-authored-by: Kaiqi Dong <kaiqi@kth.se>
Co-authored-by: Richard Shadrach <45562402+rhshadrach@users.noreply.github.com>
Co-authored-by: Simon Hawkins <simonjayhawkins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@jreback jreback Awaiting requested review from jreback

Assignees
No one assigned
Labels
Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanovmg @jreback @jbrockmendel @simonjayhawkins