Skip to content

TST/CLN: deduplicate troublesome rank values #38894

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.

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

Merged
merged 7 commits into from
Jan 4, 2021

Conversation

mzeitlin11
Copy link
Member

@mzeitlin11 mzeitlin11 added Testing pandas testing functions or related to the test suite Clean labels Jan 2, 2021
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

this is ok for now. i think if we keep adding more shared fixtures (good thing), this file might bloat, so maybe have to do something later.

cc @jbrockmendel

@@ -591,6 +593,91 @@ def narrow_series(request):
return _narrow_series[request.param].copy()


_dtype_nuisance_arr_map = {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here about where this is used

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

"object": [NegInfinity(), "1", "A", "BA", "Ba", "C", Infinity()],
}

_dtype_na_map = {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put this inside the fixture, this is generally a special case

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -591,6 +593,91 @@ def narrow_series(request):
return _narrow_series[request.param].copy()


_dtype_nuisance_arr_map = {
Copy link
Contributor

Choose a reason for hiding this comment

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

put rank in the name

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@jreback jreback added this to the 1.3 milestone Jan 3, 2021
iranks = iseries.rank()
tm.assert_series_equal(iranks, exp)
def test_rank_inf(self, nuisance_rank_series_and_expected):
series, expected = nuisance_rank_series_and_expected
Copy link
Contributor

Choose a reason for hiding this comment

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

actually an alternative here is simply to use frame_or_series fixture and leave the fixture in tests/frame/methods (e.g. this is what we do for things for both series & frame), i think in this case it makes more sense.

Copy link
Member

Choose a reason for hiding this comment

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

yah i think this makes a lot more sense than a relatively-complicated fixture in conftest

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks that is much nicer, good pattern to know

@jreback
Copy link
Contributor

jreback commented Jan 3, 2021

perfect, ping on green

@mzeitlin11
Copy link
Member Author

green

@jreback jreback merged commit eb53bf7 into pandas-dev:master Jan 4, 2021
@jreback
Copy link
Contributor

jreback commented Jan 4, 2021

thanks

@mzeitlin11 mzeitlin11 deleted the ref/rank_fixture branch January 4, 2021 03:55
luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
* WIP

* TST/CLN: deduplicate troublesome rank values

* Remove unneeded imports

* Address comments

* Use frame_or_series instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants