-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
ENH: Expose symlog scaling in plotting API #24968
ENH: Expose symlog scaling in plotting API #24968
Conversation
be9e0bd
to
2b18113
Compare
Codecov Report
@@ Coverage Diff @@
## master #24968 +/- ##
==========================================
- Coverage 92.38% 42.88% -49.5%
==========================================
Files 166 166
Lines 52398 52406 +8
==========================================
- Hits 48406 22476 -25930
- Misses 3992 29930 +25938
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24968 +/- ##
==========================================
+ Coverage 91.45% 91.47% +0.01%
==========================================
Files 172 173 +1
Lines 52892 52883 -9
==========================================
+ Hits 48373 48375 +2
+ Misses 4519 4508 -11
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.25.0.rst
Outdated
- | ||
- | ||
- | ||
:func:`DataFrame.plot` allows to expose symlog scaling for axis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this more informative, something like
DataFrame.plot keyword logy
and logx
now accept the value sym
to .....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the issue number (or if not issue, this PR number)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review, changed! @jreback
pandas/plotting/_core.py
Outdated
@@ -308,10 +308,21 @@ def _setup_subplots(self): | |||
|
|||
axes = _flatten(axes) | |||
|
|||
if self.logx or self.loglog: | |||
valid_log = [False, True, 'sym', None] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a set comprehension to check
can anyone give a feedback? ^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor comments. LGTM otherwise.
doc/source/whatsnew/v0.25.0.rst
Outdated
@@ -19,6 +19,8 @@ including other versions of pandas. | |||
Other Enhancements | |||
^^^^^^^^^^^^^^^^^^ | |||
|
|||
|
|||
- :func:`DataFrame.plot` keywords ``logy``, ``logx`` and ``loglog`` can now accept the value ``sym`` for symlog scaling. (:issue:`24867`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put sym in quotes, like ``'sim'``
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
pandas/plotting/_core.py
Outdated
valid_log = {False, True, 'sym', None} | ||
input_log = {self.logx, self.logy, self.loglog} | ||
if input_log - valid_log: | ||
raise ValueError("Valid inputs are boolean, None and 'sym'.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capture the bad parameter values and include them in the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, changed! @TomAugspurger
d7363a7
to
88705a5
Compare
pandas/plotting/_core.py
Outdated
valid_log = {False, True, 'sym', None} | ||
input_log = {self.logx, self.logy, self.loglog} | ||
if input_log - valid_log: | ||
raise ValueError(f"Valid inputs are boolean, None and 'sym'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use f-strings yet, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, changed! thanks! @TomAugspurger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May need to push these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, i am really really sorry... i thought i successfully pushed the change before rushing home @TomAugspurger
pandas/tests/plotting/test_frame.py
Outdated
# GH: 24867 | ||
df = DataFrame({'a': np.arange(100)}, index=np.arange(100)) | ||
|
||
msg = "Valid inputs are boolean, None and 'sym'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the match msg to ensure that the bad parameter is included. You can remove the "wrong_input" fixture I think, and just test one bad input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you address merge conflict?
pandas/tests/plotting/test_frame.py
Outdated
ax = df.plot(logy=True) | ||
self._check_ax_scales(ax, yaxis='log') | ||
assert ax.get_yscale() == 'log' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than these individual checks can you parametrize all of the combinations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! @WillAyd i parametrize the combinations.
@TomAugspurger Hi, Tom, I checked your comment in #25586 and made some small changes and modified the tests to align it, feel free to provide your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small doc comments
logx : bool or 'sym', default False | ||
Use log scaling or symlog scaling on x axis | ||
.. versionadded:: 0.25.0 | ||
logy : bool or 'sym' default False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a blank line after the versionadded (for each of these); make these versionchanged
@TomAugspurger a quick look if you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm as well
thanks @charlesdong1991 |
git diff upstream/master -u -- "*.py" | flake8 --diff