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

DEPR: rename offset alias ‘MS’ to ‘MB’ #56840

Open
natmokval opened this issue Jan 11, 2024 · 11 comments
Open

DEPR: rename offset alias ‘MS’ to ‘MB’ #56840

natmokval opened this issue Jan 11, 2024 · 11 comments
Labels
Deprecate Functionality to remove in pandas Frequency DateOffsets Needs Discussion Requires discussion from core team before further action

Comments

@natmokval
Copy link
Contributor

As a follow up of renaming ‘M’ to ‘ME’ for MonthEnd (#52064) I suggest to rename strings denoting MonthBegin frequency from ‘MS’ to ‘MB’.

The reason: we have alias ‘ms’ for the class Milli and using ‘MB’ for MonthBegin will be more clear. It will help to create consistent naming for aliases.

@natmokval natmokval added Deprecate Functionality to remove in pandas Frequency DateOffsets Needs Discussion Requires discussion from core team before further action labels Jan 11, 2024
@mathause
Copy link
Contributor

Might be too late but it would make the life of downstream libraries easier if this was also included in 2.2 (if you do it). FYI @spencerkclark

@mathause
Copy link
Contributor

Would that only be for month? Then it would be inconsistent with the other freq strings, which could be confusing ("MB" and "YS").

@natmokval
Copy link
Contributor Author

Would that only be for month? Then it would be inconsistent with the other freq strings, which could be confusing ("MB" and "YS").

renaming the alias for MonthBegin to "MS" is just the beginning. The whole list of aliases to change:
"YE", "BYE", "QE", "BQE", "ME", "BME", "SME", "CBME"

@natmokval
Copy link
Contributor Author

Might be too late but it would make the life of downstream libraries easier if this was also included in 2.2 (if you do it). FYI @spencerkclark

unfortunately there is not enough time to resolve this issue. The reason: after renaming "MS" to "MB" we need to rename other aliases denoting end of some time period.

@mathause
Copy link
Contributor

unfortunately there is not enough time to resolve this issue. The reason: after renaming "MS" to "MB" we need to rename other aliases denoting end of some time period.

Inconvenient but understandable.

renaming the alias for MonthBegin to "MS" is just the beginning. The whole list of aliases to change: "YE", "BYE", "QE", "BQE", "ME", "BME", "SME", "CBME"

These are already implemented for pandas 2.2, right? And these are all consistent: all use "E" for end. But changing only "MS" to "MB", will leave us with "YS", "QS", "MB", ...., which I would find confusing.

But maybe I missed/ misunderstood something.

@natmokval
Copy link
Contributor Author

natmokval commented Jan 16, 2024

unfortunately there is not enough time to resolve this issue. The reason: after renaming "MS" to "MB" we need to rename other aliases denoting end of some time period.

Inconvenient but understandable.

renaming the alias for MonthBegin to "MS" is just the beginning. The whole list of aliases to change: "YE", "BYE", "QE", "BQE", "ME", "BME", "SME", "CBME"

These are already implemented for pandas 2.2, right? And these are all consistent: all use "E" for end. But changing only "MS" to "MB", will leave us with "YS", "QS", "MB", ...., which I would find confusing.

sorry, it's my mistake. You are right, the renaming "Y", "Q", etc. to "YE", "QE", etc. is already implemented for pandas 2.2.

The list of aliases to change: “YS”, "BYS”, "QS”, "BQS”, "MS”, "BMS”, "SMS”, "CBMS”.

@aulemahal
Copy link

Hi all,
On one hand, I agree that it makes more sense to have "MB" as the class is named "MonthBegin". On the other, theses changes are very breaking for us (developpers and users of xclim). By the nature of the library, we have those strings "hardcoded" everywhere in the code, but even more so in user code.

I personally think that MS is fine as it is and that the change isn't worth the hassle. At the least, if all the breaking changes were in the same version (2.2), it would be much easier to work around!

I know our usage isn't representative of the vast majority of pandas' users, but I thought it was pertinent to raise that flag here. Thanks!

@jbrockmendel
Copy link
Member

-0, there were some of these that were a real problem, but AFAICT those have all been handled

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Jan 18, 2024

thanks all for your comments!

there kind of still is a problem though. currently:

  • pd.date_range('2000', periods=3, freq='YS') is valid (YearBegin)
  • but pd.date_range('2000', periods=3, freq='ys') is also valid, and does the same thing (YearBegin)

however:

  • pd.date_range('2000', periods=3, freq='MS') is valid (MonthBegin)
  • but pd.date_range('2000', periods=3, freq='ms') is also valid, and means something completely different (millisecond)

One solution, as Natalia's suggesting, is to rename the *S ones to *B (all in one go - it's too late for 2.2, this would have to be for the 3.x cycle if agreed)
But another solution would be to disallow mixed-casing. So, 'YS' would mean YearBegin, and 'ys' would raise. Else, if people get used to being able to pass both uppercase and lowercase, then the 'MS' / 'ms' difference is going to bite them

@aulemahal
Copy link

If I may, from my perspective disallowing mixed-casing would be a preferable option. Offsets are described in the doc with a specific case, and same case is always used in examples (AFAIK). Disallowing mixed-case is still a "regression" as it removes a "feature", but at least it does not break the documented usage of the library.

@MarcoGorelli
Copy link
Member

thanks for your input, this is useful! that may indeed be better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Frequency DateOffsets Needs Discussion Requires discussion from core team before further action
Projects
None yet
Development

No branches or pull requests

5 participants