REGR: NotImplementedError: Prefix not defined when slicing offset with loc

[CloseChoice]

• closes BUG: NotImplementedError: Prefix not defined when slicing offset with loc #46671
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/v1.4.4.rst file if fixing a bug or adding a new feature.

[pep8speaks]

Hello @CloseChoice! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-08-22 20:30:59 UTC

[CloseChoice]

The problem in here is that we expect different types in this parsing.parse_time_string which are not consistent with the rule_code attribute, e.g.
pandas/tests/series/indexing/test_datetime.py::test_getitem_setitem_periodindex where the type of self.freq is pandas._libs.tslibs.offsets.Hour. This class implements the _prefix attribute.
But in the newly added test test_getitem_iloc_dateoffset_days self.freq has the type pandas._libs.tslibs.offsets.DateOffset which does inherit the _prefix method from BaseOffset as an NotImplementedError.

Is this on purpose? Shouldn't we be able to fix this issue by just implementing the _prefix method correctly?

[phofl]

Agreed, I think we should figure out first, if _prefix is supposed to be defined

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

[jorisvandenbossche]

@CloseChoice thanks for the PR! (and sorry for that automated comment, it's not you who didn't update the PR, but it's a review that was lacking)

I merged in main to fix the conflicts

[jorisvandenbossche]

Looks good to me.

I think we can try to clean up the situation around rule_code in another PR that is not targeted for 1.4.x (should DateOffset have that defined? Should we actually pass something (greqstr/inferred_freq) to the parse function if rule_code isn't defined?)

[jorisvandenbossche]

In practice, I don't think we actually run into cases that need this. For example, freqstr for the DateOffset case gives:

In [3]: d.index.freq
Out[3]: <DateOffset: days=1>

In [4]: d.index.freqstr
Out[4]: '<DateOffset: days=1>'

This "<DateOffset: days=1>" string is never going to be useful. But in practice, it also seems that parsing.parse_time_string only actually uses freq in very few cases. From a quick look, it only seems to use it if it is exactly equal to "M", or in case of parsing quarters. Here I could create a snippet where passing the freq actually impacts the result:

In [6]: parsing.parse_time_string("4q2022", "A-DEC")
Out[6]: (datetime.datetime(2022, 10, 1, 0, 0), 'quarter')

In [7]: parsing.parse_time_string("4q2022", "A-NOV")
Out[7]: (datetime.datetime(2022, 9, 1, 0, 0), 'quarter')

But, so this only is for quarterly offsets, and those do define a rule_code, so you probably don't run into this issue with that kind of freqs.

This line freq = getattr(self, "freqstr", getattr(self, "inferred_freq", None)) is restoring what we did before #42149, so that's good for a regression fix in the bug-fix release. But we should maybe also see if we can just remove it later on (and just pass None here if rule_code isn't defined?)

[CloseChoice]

I checked passing None if rule_code isn't defined and a couple of tests fail in this case. Didn't look into it further though. But I think it should be possible to improve the situation around rule_code.

[CloseChoice]

Looks good to me.

I think we can try to clean up the situation around rule_code in another PR that is not targeted for 1.4.x (should DateOffset have that defined? Should we actually pass something (greqstr/inferred_freq) to the parse function if rule_code isn't defined?)

First things first: Thanks once again for helping out and the very friendly tone of your comments. I really appreciate it.

Yes, I totally agree that we should clean up rule_code at some point, I am also very willing to dive into that.
When I investigated the situation around rule_code there are a couple of things we could do:

1. One thing that came to my mind is to add rule_code for DateOffsets. This would help us get rid of the except part in this PR
2. Refactor parse_date_string to not need this information at all. As you mentioned in the other freq is actually used in very limited cases (as I can see, it only used in the chain parse_time_string -> parse_datetime_string_with_reso -> _parse_dateabbr_string -> quarter_to_myear -> get_rule_month and in _parse_dateabbr_string where it is checked against M. So there might be a good way of working around this by finding the rule_month (as given by get_rule_month) and checking for M first and implementing a special case for this somewhere not so deep down in the chain.

There are also a couple todos written in the code, so I think we could improve here quite some things.

[simonjayhawkins]

Thanks @CloseChoice typo to fix otherwise lgtm

[jorisvandenbossche]

Thanks @CloseChoice !

When I investigated the situation around rule_code there are a couple of things we could do:

Could you move that summary to a new issue to track this?