Skip to content
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: add is_leap_year property for datetime-like #13739

Closed
wants to merge 1 commit into from

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Jul 21, 2016

  • closes DEPR/ENH: isleapyear #13727
  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry
  • found there is tslib.isleapyear also, but i don't think it is public (deprecated also).
  • pd.NaT.is_leap_year results in False, as I think users want bool array.

@sinhrks sinhrks added Enhancement Datetime Datetime data dtype Deprecate Functionality to remove in pandas labels Jul 21, 2016
@sinhrks sinhrks added this to the 0.19.0 milestone Jul 21, 2016
@sinhrks sinhrks changed the title ENH: add is_leapyear property for datetime-like ENH: add is_leap_year property for datetime-like Jul 21, 2016
@jreback
Copy link
Contributor

jreback commented Jul 21, 2016

lgtm

@@ -332,6 +332,7 @@ API changes
- ``PeriodIndex.fillna`` with ``Period`` has different freq now coerces to ``object`` dtype (:issue:`13664`)
- More informative exceptions are passed through the csv parser. The exception type would now be the original exception type instead of ``CParserError``. (:issue:`13652`)
- ``astype()`` will now accept a dict of column name to data types mapping as the ``dtype`` argument. (:issue:`12086`)
- ``Timestamp``, ``Period``, ``DatetimeIndex``, ``PeriodIndex`` and ``.dt`` accessor has ``.is_leap_year`` property to return logical whether date belong to leap year. (:issue:`13727`)
Copy link
Member

Choose a reason for hiding this comment

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

has -> have
return logical -> check ? (sounds better IMO)
belong -> belongs
leap year -> a leap year

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, github is not really made for such linguistic review, would be much easier if I could just directly edit it .. :-)

@jreback
Copy link
Contributor

jreback commented Jul 21, 2016

add to docs here as well.

note when looking at them again

is_year_start
is_year_end
is_leap_year

looks a tiny bit weird.....but makes sense

return is_leapyear(year)


cpdef isleapyear_arr(ndarray years):
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this private? (_isleapyear_arr) Because if we regard functions here as internal, we actually don't need to deprecate isleapyear and could just remove it

@sinhrks
Copy link
Member Author

sinhrks commented Jul 21, 2016

@jorisvandenbossche @jreback Thx for kind reviews always:) Fixed points.

@codecov-io
Copy link

codecov-io commented Jul 21, 2016

Current coverage is 85.32% (diff: 100%)

Merging #13739 into master will increase coverage by <.01%

@@             master     #13739   diff @@
==========================================
  Files           141        141          
  Lines         50679      50686     +7   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43240      43247     +7   
  Misses         7439       7439          
  Partials          0          0          

Powered by Codecov. Last update 474fd05...5d227ee

@@ -4822,8 +4833,23 @@ def dates_normalized(ndarray[int64_t] stamps, tz=None):
#----------------------------------------------------------------------

def isleapyear(int64_t year):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any reason to deprecate this; by-definition its internal if in tslib.pyx

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah i don't think users use it... simply removed.

@sinhrks sinhrks force-pushed the is_leapyear branch 3 times, most recently from 208a6a7 to f1a2098 Compare July 24, 2016 14:30
@jreback
Copy link
Contributor

jreback commented Jul 24, 2016

lgtm. merge on pass.

@jreback
Copy link
Contributor

jreback commented Jul 24, 2016

FYI depending on when you rebased, py3 may fail for this

@@ -609,7 +611,9 @@ Deprecations
- ``as_recarray`` has been deprecated in ``pd.read_csv()`` and will be removed in a future version (:issue:`13373`)
- top-level ``pd.ordered_merge()`` has been renamed to ``pd.merge_ordered()`` and the original name will be removed in a future version (:issue:`13358`)
- ``Timestamp.offset`` property (and named arg in the constructor), has been deprecated in favor of ``freq`` (:issue:`12160`)
- ``pivot_annual`` is deprecated. Use ``pivot_table`` as alternative, an example is :ref:`here <cookbook.pivot>` (:issue:`736`)
- ``pd.tseries.util.pivot_annual`` is deprecated. Use ``pivot_table`` as alternative, an example is :ref:`here <cookbook.pivot>` (:issue:`736`)
- ``pd.tseries.util.isleapyear`` and ``pd.tslib.isleapyear`` have been deprecated and will be removed in a subsequent release. Datetime-likes now have a ``.is_leap_year`` property. (:issue:`13727`)
Copy link
Member

@jorisvandenbossche jorisvandenbossche Jul 24, 2016

Choose a reason for hiding this comment

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

The tslib.isleaypear is just deleted I think in the latest revision instead of deprecated, so we should remove it here

@jreback
Copy link
Contributor

jreback commented Jul 25, 2016

thanks @sinhrks
nice PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Deprecate Functionality to remove in pandas Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPR/ENH: isleapyear
4 participants