Skip to content

ENH: GH11128 add weekday_name to DatetimeIndex and .dt #11813

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

Closed
wants to merge 1 commit into from

Conversation

bahrunnur
Copy link

closes #11128

some examples:

In [5]: s2 = s.to_frame('value').assign(weekday=s.index.weekday_name)

In [6]: s2
Out[6]:
            value    weekday
2015-09-01      0    tuesday
2015-09-02      1  wednesday
2015-09-03      2   thursday
2015-09-04      3     friday
2015-09-05      4   saturday
2015-09-06      5     sunday
2015-09-07      6     monday
2015-09-08      7    tuesday
2015-09-09      8  wednesday
2015-09-10      9   thursday

and

In [7]: s3 = pd.Series(pd.date_range('2015-09-01', '2015-09-10'))

In [8]: s3.dt.weekday_name
Out[8]:
0      tuesday
1    wednesday
2     thursday
3       friday
4     saturday
5       sunday
6       monday
7      tuesday
8    wednesday
9     thursday
dtype: object

Please review my code 😄

@jreback jreback added this to the 0.18.0 milestone Dec 10, 2015
@jreback jreback added Enhancement Datetime Datetime data dtype labels Dec 10, 2015
@@ -29,7 +29,7 @@ def setUp(self):
def test_ops_properties(self):
self.check_ops_properties(['year','month','day','hour','minute','second','weekofyear','week','dayofweek','dayofyear','quarter'])
self.check_ops_properties(['date','time','microsecond','nanosecond', 'is_month_start', 'is_month_end', 'is_quarter_start',
'is_quarter_end', 'is_year_start', 'is_year_end'], lambda x: isinstance(x,DatetimeIndex))
'is_quarter_end', 'is_year_start', 'is_year_end', 'weekday_name'], lambda x: isinstance(x,DatetimeIndex))
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an explict test for the results of this (you can do it in tseries/test_timeseries.py,
e.g. create a datetimeindex and assert that i.weekday_name matches your expectation, e.g. since you are lowercasing

Copy link
Contributor

Choose a reason for hiding this comment

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

furthermore test with some NaT in there (these should be np.nan, which the masker will fix for you)

Copy link
Author

Choose a reason for hiding this comment

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

ok, I'm on it 💪

@kawochen
Copy link
Contributor

The first letters should be capitalized?

"""
weekdays = [calendar.day_name[i].lower() for i in range(7)]
return self._maybe_mask_results(_algos.arrmap_object(self.asobject.values, lambda x: weekdays[x.weekday()]))

Copy link
Contributor

Choose a reason for hiding this comment

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

You are going to want to do something like this as what you are doing is going to be much much slower (only reason is this is done for .date is that its creating python objects so no way around it).

In [25]: index = pd.date_range('20130101 09:00:00',periods=20)

In [26]: weekdays = [calendar.day_name[i].lower() for i in range(7)]

In [27]: pandas.core.common.take_nd(np.array(weekdays),index.weekday)
Out[27]: 
array(['tuesday', 'wednesday', 'thursday', 'friday', 'saturday', 'sunday',
       'monday', 'tuesday', 'wednesday', 'thursday', 'friday', 'saturday',
       'sunday', 'monday', 'tuesday', 'wednesday', 'thursday', 'friday',
       'saturday', 'sunday'], 
      dtype='|S9')

then return this an an object array
you may need to do com._ensure_int64 on the i.weekday (I don't remember)

Copy link
Author

Choose a reason for hiding this comment

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

I'm totally noob about tslib.pyx because it is a Cython code that interacting with numpy, CPython datetime. I wanna give my best shot to update the implementation on the tslib.pyx. And then, acces it via _field_accessor in tseries/index.py with additional if branching checking if it is requesting a weekday_name. And we also can add like month_name perhaps? What do you think @jreback ?

@jreback
Copy link
Contributor

jreback commented Dec 10, 2015

pls also add this to Timestamp.

@jorisvandenbossche
Copy link
Member

Sorry I did not comment on the original issue before you put effort in this, but I wonder a bit if this is worth a new method (we already have many many methods, so adding a new needs a strong case I think).
Wouldn't be some good docs be sufficient? Eg specifically add in the docstring that if you want to get the names instead of the numbers, how you can do that most easily?

@jreback
Copy link
Contributor

jreback commented Dec 11, 2015

I actually thing this is pretty reasonable. Its an adjust method for weekday that is useful. No harm IMHO at trivially expanding the API.

@bahrunnur
Copy link
Author

@jorisvandenbossche that's ok, this is my first time doing PR on pandas and I learn a lot about the code and contributing process, etc. What I think about it is a 'nice to have' feature and it has Difficulty Novice label that I think this is a perfect issue for me to learn more about the underlying structure of pandas. IMHO I have not found a strong usage on this feature. But, @jreback may have another perspective about it.

What I think about the usage of it is on the REPL like IPython or just python to check thing that what we are intended to do on a date are doing exactly what we want. For example, in #11123 if we can see the day name seamlessly without interpreting the number into day name in our mind. Perhaps it could help our mind a little bit 😂

@kawochen Hmm yeah, I'm just thinking that the capitalized form may be useful for serialization into csv, excel, etc. Should I change to capitalized form @jreback? I need some advice to choose between lower case or capitalized 🙏.

@jreback
Copy link
Contributor

jreback commented Dec 11, 2015

prob should use what calendar gives back for the names

and, this actually could/should be implemented similarly to the _field_accessors (e.g. directly in cython).

@bahrunnur bahrunnur force-pushed the weekday-name branch 3 times, most recently from 9142992 to f5bb183 Compare December 11, 2015 16:34
@jreback
Copy link
Contributor

jreback commented Jan 6, 2016

@bahrunnur can you rebase / update

@jreback
Copy link
Contributor

jreback commented Jan 20, 2016

I still like this. @bahrunnur can you rebase / update

@bahrunnur
Copy link
Author

Oh hi @jreback !
I'm sorry for the slow response. I've been on vacation since the first day of this year. Now, I have rebased my code.


if field == 'weekday_name':
for i in range(count):
if dtindex[i] == NPY_NAT: out[i] = -1; continue
Copy link
Contributor

Choose a reason for hiding this comment

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

put np.nan in if its NaT

Copy link
Contributor

Choose a reason for hiding this comment

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

needs some tests for NaT in the data as well (and test pd.NaT) as well

Choose a reason for hiding this comment

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

We're returning strings, isn't an empty string better for NaT? or str(np.nan)?
Where do we best put tests with NaT? Maybe just add a case to an existing test? How do I run these tests, where is the beginning of all this spagetti? (bear with me, I'm on a steep curve).

Copy link
Contributor

Choose a reason for hiding this comment

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

no np.nan is the empty indicator in non-datetimelike objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

tests go in tseries/tests_timeseries.py look where things like is_year_end properties are tested

@jreback
Copy link
Contributor

jreback commented Jan 30, 2016

can you update

@jreback jreback modified the milestones: Next Major Release, 0.18.0 Feb 2, 2016
@jreback
Copy link
Contributor

jreback commented Mar 12, 2016

can you rebase/update

@jreback jreback removed this from the Next Major Release milestone Mar 13, 2016
@jreback
Copy link
Contributor

jreback commented Apr 5, 2016

closed in favor of #12803

@jreback jreback closed this Apr 5, 2016
jreback pushed a commit that referenced this pull request Apr 26, 2016
closes #11128   replaces #11813     - [x] tests added / passed   - [x]
passes ``git diff upstream/master | flake8 --diff``   - [x] whatsnew
entry    Completed merge of Bahrunnur's PR 'weekday-name'
Moved 'enhancements' to v0.18.1.txt          Added tests to
pandas/tests/series/test_datetime_values.py          Added
weekday_name property producing 'NaT' to NaTType class

Author: Bastiaan <[email protected]>
Author: Bahrunnur <[email protected]>

Closes #12803 from BastiaanBergman/ENH11128 and squashes the following commits:

c579d71 [Bastiaan] Tiny fixes as requested by jreback, GH12803.
6f246d5 [Bastiaan] Small fixes as requested by jreback, GH12803.
7b14d5c [Bahrunnur] ENH: GH11128 add weekday_name to DatetimeIndex and .dt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: add weekday_name to DatetimeIndex and .dt
5 participants