More informative exception when trying to use MS as period frequency
#5340
Conversation
|
I'm not convinced this is reasonable to do. I don't get a keyerror when using "MS" - I think it's just a backwards compatibility thing where MS is always made into milliseconds (which is what "L" means too). |
|
@jtratner I thought you had originally been seeing the key error? When I use Pandas v0.12, I definitely am getting key error. The ability to use MS to mean milliseconds is inconsistent. There are parts of the Pandas code that explicitly prevent this and others that do not. |
|
@cancan101 seems like the underlying problem is that this fails poorly: pd.Period('2013').asfreq(freq='L')Also, Wakari version of pandas gets a |
|
I actually think there are two issues here. One is that MS, in my opinion, On Sunday, October 27, 2013, Jeff Tratner wrote:
|
|
Prefer to address both. Also, what should MS map to, if anything? |
|
Actually, what I'd really prefer is that passing an unknown frequency raises |
|
Initially, I thought that |
| initial = Period("2013") | ||
|
|
||
| self.assertEqual(initial.asfreq(freq="M", how="S"), Period('2013-01', 'M')) | ||
| self.assertRaises(ValueError, initial.asfreq, freq="MS", how="S") |
There was a problem hiding this comment.
please change one of these to have (a portion of) the text of the error message. Much easier to maintain that way (and adds really minimal performance hit).
|
@jtratner That commit does not fix this issue. |
|
Or I should say, it solves the issue by having the period 'MS' map to 'L' correctly which I would argue is incorrect. |
|
Ah, okay, I didn't realize, let's reopen this then. |
|
Probably the other should have said 'related to #5340' and fixes for the other PR. |
|
Just FYI - when you put " |
|
Sorry, this was my fault. I meant #5430 and mis-typed in the commit. |
|
FWIW, from what I remember when this code was being written some years (!) ago, MS as a frequency (this is before the period concept existed in pandas) was meant to map to MonthBegin. It looks to me like MS -> milliseconds was added when it was decided that pandas would provide most/all of the capabilities that were in scikits.timeseries and they were using the mapping MS -> milliseconds. This PR looks like what I would also do to revert back to some consistency for MS. I'll check it out locally and let you know if I notice anything from the statsmodels usage side and my code. |
|
@jseabold What are your thoughts on this PR? |
|
Can you explain again why this shouldn't work? |
|
Nevermind. I see that using 'M' actually returns a start of the month timestamp. Odd. Yeah, this looks fine to me. At least it clears up the problem with the milliseconds. |
|
@cancan101 where exactly is the inconsistency in error messages? |
|
@cancan101 can you rebase and squash. Is this dependent on the offsets/period cleanup? |
|
I do not believe this is dependent on the other PR.
|
|
merged via 0b7e917 |
Closes #5332