-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
Conversation
@@ -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)) |
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 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
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.
furthermore test with some NaT
in there (these should be np.nan
, which the masker will fix for you)
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.
ok, I'm on it 💪
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()])) | ||
|
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.
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)
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.
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 ?
pls also add this to |
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). |
I actually thing this is pretty reasonable. Its an adjust method for weekday that is useful. No harm IMHO at trivially expanding the API. |
@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 🙏. |
prob should use what and, this actually could/should be implemented similarly to the |
9142992
to
f5bb183
Compare
@bahrunnur can you rebase / update |
I still like this. @bahrunnur can you rebase / update |
f5bb183
to
e652e91
Compare
Oh hi @jreback ! |
|
||
if field == 'weekday_name': | ||
for i in range(count): | ||
if dtindex[i] == NPY_NAT: out[i] = -1; continue |
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 np.nan
in if its NaT
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.
needs some tests for NaT
in the data as well (and test pd.NaT
) as well
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'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).
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.
no np.nan
is the empty indicator in non-datetimelike objects.
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.
tests go in tseries/tests_timeseries.py
look where things like is_year_end
properties are tested
can you update |
can you rebase/update |
closed in favor of #12803 |
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
closes #11128
some examples:
and
Please review my code 😄