Skip to content

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.

- :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