GH1089 Migrate frame/series tests to new framework #1093
Conversation
There was a problem hiding this comment.
For the ones where the results aren't right (mostly the arithmetic ones), the PR should include the fixes to the PYI files to make those results come out right. I don't want to approve the test changes when the results aren't what we expect.
Could include the fix for complex here as well (even though you created a separate issue)
|
Will address the changes in this PR (although I created a new issue), seems more correct but agree that it will be slightly larger to review. |
…1089_tests_migration
You have 2 options:
I'm fine either way, although I think it would be easier to do (1). No issue for me in reviewing that large of a PR. |
There was a problem hiding this comment.
I think I figured out your issues by pulling your PR and trying things. I only tested with pyright in VS Code, so when you run the full tests, something might turn up with mypy.
Also - a request - can you NOT resolve conversations. I will do it once I see that my comment has been resolved.
There was a problem hiding this comment.
For safety, we should duplicate the tests in test_types_element_wise_arithmetic() but where s and s2 have parameters dtype=float. Then the results should all be Series[float]
Also, worth doing a set where s is Series[int] and s2 is Series[float] and vice versa.
And we should do ones with the following. If you create a DataFrame that contains numbers, let's say with a column named s3 then df["s3"] is Series[Unknown] in pyright, So now we want to test the arithmetic operations between Series[int] and df["s3"], Series[float] and df["s3"] and df["s3"] and df["s4"] (where s4 is another column of numbers). You'd have to do the tests with all the symmetries.
So create a bunch of new copies of test_types_element_wise_arithmetic() (with different names) that test these different combinations as the first and second operands of the arithmetic operation, where Series[Unknown] comes from a column in a DataFrame
Series[int]andSeries[float]Series[float]andSeries[int]Series[float]andSeries[float]Series[int]andSeries[Unknown]Series[Unknown]andSeries[int]Series[Unknown]andSeries[Unknown]Series[float]andSeries[Unknown]Series[Unknown]andSeries[float]
It might be worth just moving test_types_element_wise_arithmetic() into a new test_series_arithmetic.py file and just put all the new 8 versions there.
Except for division, the results involving Series[Unknown] should just be Series
|
I haven't added all the tests yet, this is quite a bit of work so will probably delay it to another PR and fix in priority based on if it causes more issues for the users. |
I'm a bit hesitant to approve without the complete set of tests, because people do I know it's a lot of work and really appreciate your hard work on this! |
There was a problem hiding this comment.
Found a couple of things. As mentioned at #1093 (comment), I'd like to have the full set of tests included.
Once that is due, then I will take another full look at the PR - I've only been looking at your recent changes each time you push.
It seems pretty close!
tests/test_series_arithmetic.py
Outdated
| s2 = pd.Series([0, 1, -105]) | ||
|
|
||
| check(assert_type(s + s2, pd.Series), pd.Series) | ||
| check(assert_type(s.add(s2, fill_value=0), "pd.Series[float]"), pd.Series) |
There was a problem hiding this comment.
should be pd.Series - type should be unknown
tests/test_series_arithmetic.py
Outdated
|
|
||
|
|
||
| def test_element_wise_int_unknown() -> None: | ||
| s = cast(pd.Series, pd.Series([7, -5, 10])) |
There was a problem hiding this comment.
Can you do this as follows:
df = pd.DataFrame({"s": pd.Series{[7, -5, 10])})
s = df["s"]Because that's a common pattern in pandas usage and will test the "unknown" usage.
|
@Dr-Irv almost there (sorry was out on a break), I am just having a hard time wrapping up the int/float on the left and Unknown on the right. Do you have an idea how to take that in consideration? Thanks! |
I found something that worked for The idea is that when you do If we do that, ideally, we should change any method in all PYI files (and the associated tests) that has It might be worth to do a separate PR first that makes the change to do I've placed those simple changes to make Let me know if you're willing to give the |
|
Got it, that makes sense! I did not want to go that route too much originally since it was conflicting with the following issue #718 but I will give it a try, I think it should work as you recommended it! |
assert_type()to assert the type of any return value