Skip to content

ENH: is_scalar returns True for DateOffset objects #18982

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

Merged
merged 1 commit into from
Dec 29, 2017
Merged

ENH: is_scalar returns True for DateOffset objects #18982

merged 1 commit into from
Dec 29, 2017

Conversation

databasedav
Copy link
Contributor

@databasedav databasedav commented Dec 29, 2017

I've implemented is_offset identically to is_period_object and added it to isscalar and is_scalar is now returning true for DateOffsets.

But not sure how to go about this exactly:

is_offset should also be imported / tested in pandas/core/dtypes/common.py

The other "is" functions that are explicitly imported from inference is_string_like and is_list_like are used for testing but don't look like they themselves are being tested so I'm not sure what kind of test is needed. The rest of the "is" functions are imported with a wildcard but pylint is telling me they are not used (should I go through and make the required imports explicit?).

@codecov
Copy link

codecov bot commented Dec 29, 2017

Codecov Report

Merging #18982 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #18982   +/-   ##
=======================================
  Coverage   91.58%   91.58%           
=======================================
  Files         150      150           
  Lines       48967    48967           
=======================================
  Hits        44846    44846           
  Misses       4121     4121
Flag Coverage Δ
#multiple 89.94% <ø> (ø) ⬆️
#single 41.73% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4883a43...417bb3a. Read the comment docs.

@@ -144,6 +144,7 @@ Other Enhancements
- :class:`Interval` and :class:`IntervalIndex` have gained a ``length`` attribute (:issue:`18789`)
- ``Resampler`` objects now have a functioning :attr:`~pandas.core.resample.Resampler.pipe` method.
Previously, calls to ``pipe`` were diverted to the ``mean`` method (:issue:`17905`).
- :func:`is_scalar` now returns True for DateOffset objects (:issue:`18943`).
Copy link
Member

Choose a reason for hiding this comment

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

  • :func:`is_scalar` --> :func:`~pandas.api.types.is_scalar`
  • True --> ``True``
  • DateOffset --> ``DateOffset``


"""

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this white space?

@jschendel
Copy link
Member

Thanks!

It looks like the test for is_scalar in pandas/tests/dtypes/test_inference.py should be updated, specifically here:

def test_isscalar_pandas_scalars(self):
assert is_scalar(Timestamp('2014-01-01'))
assert is_scalar(Timedelta(hours=1))
assert is_scalar(Period('2014-01-01'))

Might be nice to add an assert statement for Interval in addition to DateOffset, since it should also return True but appears to be untested.

Not 100% sure about your other questions, so I'll let someone else field those instead of potentially saying something that's not right.

@databasedav
Copy link
Contributor Author

Thanks. I should be more clear about the "is" stuff, here is the wildcard import:

from .inference import is_string_like, is_list_like
from .inference import * # noqa

which I don't think conforms to PEP 8 (?). Just wondering if it's there for a reason or if I should go through and only explicitly export the necessary functions.

Also quick noobie question: when I make changes to pandas locally, I've been running python setup.py build_ext --inplace -j 4 for my changes to be reflected in the next import pandas. Is this the correct way to go about it?

@databasedav
Copy link
Contributor Author

Excessively messed around with rebasing just now, sorry about that!

@jreback
Copy link
Contributor

jreback commented Dec 29, 2017

@gitavi you don't need to rebase. we squash on merge.

@jreback jreback added this to the 0.23.0 milestone Dec 29, 2017
@jreback jreback merged commit 1bbd77a into pandas-dev:master Dec 29, 2017
@jreback
Copy link
Contributor

jreback commented Dec 29, 2017

thanks @gitavi

@jreback
Copy link
Contributor

jreback commented Dec 29, 2017

the from .inference import * is more a convenience thing, it was designed this way.

what I would like to change though is to rename lib.isscalar to lib.is_scalar, for consistency and update the doc-string to be a bit more consistent (e.g. we don't need the 'instances of ....') everything is an instance of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_scalar should return True for DateOffset objects
3 participants