GH1147 Drop version restriction on matplotlib and prevent test to write to root folder #1148
Conversation
…te to root folder
| check( | ||
| assert_type( | ||
| grouped.boxplot( | ||
| subplots=False, rot=45, fontsize=12, figsize=(8, 10), vert=False |
There was a problem hiding this comment.
Switching this to orientation='horizontal' creates an error, needs to dig into it.
There was a problem hiding this comment.
error in pytest or in typing? If pytest, then if you can reproduce by just using pandas, then report a pandas issue.
There was a problem hiding this comment.
It is a runtime error, if you try and run the code it will throw an error.
Right now with vert=False then it runs fine but changing to orientation=‘horizontal’ then it will throw an error. I need to step into the code to understand why the conversion they are recommending is actually broken in this particular case.
There was a problem hiding this comment.
This seems a pandas bug, I am going to do a little bit of investigation, it happens because the orientation parameter is not passed properly.
For now if you run the plot and there are no other Axes available this will work so I will split the test and ship it as is not to block this PR.
There was a problem hiding this comment.
I dug into it and I was not able to reproduce it on the main branch of pandas.
There was a problem hiding this comment.
So you could keep the test in there and surround it with if not PD_LTE_22, and just have the test as you modified it here, and add the new test with the if statement that would then be executed in the nightly tests (which we have to separately fix)
There was a problem hiding this comment.
Actually that would not work since the issue was how pandas passed the information to matplotlib (and we upgraded matplotlib version) and we don't have a test of the matplotlib version if I am correct.
There was a problem hiding this comment.
The test was originally failing because of the matplotlib version that was upgraded and then it was how pandas supplied the kwargs to matplotlib.
There was a problem hiding this comment.
Yes, so in the current form, with the upgraded matplotlib, the test using orientation='horizontal', the test fails with pandas 2.2.3, but works with the version in main (larger than 2.2.x).
So you could surround your desired test that includes orientation='horizontal' with a if not PD_LTE_22 code, and leave in the test as you wrote it in this PR without the if not PD_LTE_22, with a comment to remove that test once pandas releases a version beyond 2.2.
There was a problem hiding this comment.
Sorry I completely missed your point. This is addressed. I kept the split test and added for both a check on PD_LTE_22 with the new orientation="horizontal", I tested with poetry run poe pytest --nightly and it passed for the test_plotting.py tests (we have other tests failing but this is addressed in another PR #1152).
pyproject.toml
Outdated
| @@ -43,7 +43,7 @@ pyright = ">=1.1.396" | |||
| poethepoet = ">=0.16.5" | |||
| loguru = ">=0.6.0" | |||
| typing-extensions = ">=4.4.0" | |||
| matplotlib = ">=3.5.1,<3.9.0" # TODO https://github.com/pandas-dev/pandas/issues/58851 | |||
| matplotlib = ">=3.5.10" # TODO https://github.com/pandas-dev/pandas/issues/58851 | |||
There was a problem hiding this comment.
pandas now requires 3.6.3, so can update this constraint to that version, and remove the comment since that issue is now closed
There was a problem hiding this comment.
I will adjust this is a good point.
pyproject.toml
Outdated
| @@ -231,7 +231,7 @@ filterwarnings = [ | |||
| # treat warnings as errors | |||
| "error", | |||
| # until there is a new dateutil release: github.com/dateutil/dateutil/pull/1285 | |||
| "ignore:datetime.datetime.utc:DeprecationWarning", | |||
| # "ignore:datetime.datetime.utc:DeprecationWarning", | |||
There was a problem hiding this comment.
If commenting this out works, you can just remove this line and the line above
There was a problem hiding this comment.
I tried for testing since that is still a draft PR but indeed will delete.
| check( | ||
| assert_type( | ||
| grouped.boxplot( | ||
| subplots=False, rot=45, fontsize=12, figsize=(8, 10), vert=False |
There was a problem hiding this comment.
error in pytest or in typing? If pytest, then if you can reproduce by just using pandas, then report a pandas issue.
…te to root folder
|
/pandas_nightly |
| check( | ||
| assert_type( | ||
| grouped.boxplot( | ||
| subplots=False, rot=45, fontsize=12, figsize=(8, 10), vert=False |
There was a problem hiding this comment.
So you could keep the test in there and surround it with if not PD_LTE_22, and just have the test as you modified it here, and add the new test with the if statement that would then be executed in the nightly tests (which we have to separately fix)
|
/pandas_nightly |
assert_type()to assert the type of any return value