Skip to content

Conversation

@tarsur909
Copy link

@tarsur909 tarsur909 commented Jul 29, 2023

@tarsur909 tarsur909 changed the title fix hardcoded scatter marker size issue #54204 BUG: fix hardcoded scatter marker size issue #54204 Jul 29, 2023
# This means that if rcParams are temporarily changed,
# the marker size changes as well according to mpl.rc_context().
if mpl.rcParams["_internal.classic_mode"]:
s = 20
Copy link
Member

Choose a reason for hiding this comment

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

I think the default should still be 20 but deprecated in favor of using the mpl.rcParams["lines.markersize"]

Copy link
Author

@tarsur909 tarsur909 Jul 31, 2023

Choose a reason for hiding this comment

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

Hey @mroeschke thanks for your review. Could you please clarify what needs to be fixed? Should I raise a deprecation warning if mpl.rcParams["_internal.classic_mode"] is True or remove this conditional altogether? If that is not what you mean, could you please elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

The conditional should probably be something like

if s is None:
    warning.warning("The default of s=20 is deprecated and will change to mpl.rcParams["lines.markersize"]
                                 " in a future version. Specify `s` to suppress this warning",
                                 DeprecationWarning, stacklevel=find_stack_level())

Copy link
Author

@tarsur909 tarsur909 Aug 1, 2023

Choose a reason for hiding this comment

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

Hey @mroeschke , makes sense. Made the necessary changes.

@mroeschke mroeschke added the Visualization plotting label Jul 31, 2023
for method_name in dir(air_quality.plot)
if not method_name.startswith("_")
]
import warnings
Copy link
Member

Choose a reason for hiding this comment

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

You can add an :okwarning: below the ipython directive instead of this

for a, b in zip(s.plot.bar().get_xticklabels(), expected)
)

@pytest.mark.filterwarnings("default")
Copy link
Member

Choose a reason for hiding this comment

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

- Bug in :meth:`Series.plot` when invoked with ``color=None`` (:issue:`51953`)
- Fixed UserWarning in :meth:`DataFrame.plot.scatter` when invoked with ``c="b"`` (:issue:`53908`)
-
- Fixed bug in :meth:`DataFrame.plot.scatter` wherein marker size was previously hardcoded to a default value (:issue:`54204`)
Copy link
Member

Choose a reason for hiding this comment

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

Should be a note in the deprecation section now

# the marker size changes as well according to mpl.rc_context().
warnings.warn(
"""The default of s=20 is deprecated and
has changed to mpl.rcParams['lines.markersize'].
Copy link
Member

Choose a reason for hiding this comment

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

  1. Single quotes
  2. has changed -> will be changed

"""The default of s=20 is deprecated and
has changed to mpl.rcParams['lines.markersize'].
Specify `s` to suppress this warning""",
DeprecationWarning,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DeprecationWarning,
FutureWarning,

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Sep 1, 2023
@mroeschke
Copy link
Member

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

@mroeschke mroeschke closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Scatter plot has a hardcoded default value

2 participants