Skip to content

GH1147 Drop version restriction on matplotlib and prevent test to write to root folder #1148

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

Merged
merged 3 commits into from
Mar 10, 2025
Merged

GH1147 Drop version restriction on matplotlib and prevent test to write to root folder #1148

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ec6308
Gh1147 Drop version restriction on matplotlib and prevent test to wri…
loicdiridollou Mar 7, 2025
849a69b
Gh1147 Drop version restriction on matplotlib and prevent test to wri…
loicdiridollou Mar 8, 2025
86dc4e6
GH1147 PR Feedback
loicdiridollou Mar 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

pandas now requires 3.6.3, so can update this constraint to that version, and remove the comment since that issue is now closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I will adjust this is a good point.

pre-commit = ">=2.19.0"
black = ">=23.3.0"
isort = ">=5.12.0"
Expand Down Expand Up @@ -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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If commenting this out works, you can just remove this line and the line above

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried for testing since that is still a draft PR but indeed will delete.

]

# Next line needed to avoid poetry complaint
Expand Down
6 changes: 4 additions & 2 deletions tests/test_frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3996,8 +3996,10 @@ def test_hashable_args() -> None:
df.columns = ["test"] # type: ignore[assignment]

testDict = {"test": 1}
df.to_string("test", col_space=testDict)
df.to_string("test", col_space={"test": 1})

with ensure_clean() as path:
df.to_string(path, col_space=testDict)
df.to_string(path, col_space={"test": 1})


# GH 906
Expand Down
27 changes: 17 additions & 10 deletions tests/test_plotting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,10 @@ def test_plot_keywords(close_figures):

df = pd.DataFrame(np.random.rand(10, 5), columns=["A", "B", "C", "D", "E"])
check(
assert_type(df.plot(kind="box", vert=False, positions=[1, 4, 5, 6, 8]), Axes),
assert_type(
df.plot(kind="box", orientation="vertical", positions=[1, 4, 5, 6, 8]),
Axes,
),
Axes,
)

Expand Down Expand Up @@ -603,15 +606,19 @@ def test_grouped_dataframe_boxplot(close_figures):
check(assert_type(grouped.boxplot(subplots=True), Series), Series)

# a single plot
check(
assert_type(
grouped.boxplot(
subplots=False, rot=45, fontsize=12, figsize=(8, 10), vert=False
Copy link
Member Author

Choose a reason for hiding this comment

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

Switching this to orientation='horizontal' creates an error, needs to dig into it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

error in pytest or in typing? If pytest, then if you can reproduce by just using pandas, then report a pandas issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dug into it and I was not able to reproduce it on the main branch of pandas.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

The test was originally failing because of the matplotlib version that was upgraded and then it was how pandas supplied the kwargs to matplotlib.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

),
Axes,
),
Axes,
)
# check(
# assert_type(
# grouped.boxplot(
# subplots=False,
# rot=45,
# fontsize=12,
# figsize=(8, 10),
# orientation="horizontal",
# ),
# Axes,
# ),
# Axes,
# )

# not a literal bool
check(assert_type(grouped.boxplot(subplots=bool(0.5)), Union[Axes, Series]), Series)
Expand Down