CLN: enforced the deprecation of strings 'H', 'BH', 'CBH' in favor of 'h', 'bh', 'cbh' #59143

natmokval · 2024-06-28T20:21:02Z

Enforced deprecation of uppercase strings ‘H’, ‘BH’, ‘CBH’ for offsets frequencies, resolution abbreviations, and units in favor of 'h', 'bh', 'cbh'

…f ‘h’, ‘bh’, ‘cbh’

mroeschke · 2024-07-01T19:13:20Z

pandas/tests/indexes/datetimes/test_datetime.py

-            freq="cbh",
-        )
+    @pytest.mark.parametrize("freq", ["2H", "2BH", "2S"])
+    def test_CBH_deprecated(self, freq):


Suggested change

def test_CBH_deprecated(self, freq):

def test_CBH_raises(self, freq):

Thanks @mroeschke for reviewing this PR. I changed the name for the test.

mroeschke · 2024-07-01T19:14:23Z

pandas/_libs/tslibs/dtypes.pyx

@@ -362,6 +359,8 @@ cdef dict c_PERIOD_AND_OFFSET_DEPR_FREQSTR = {
    "MIN": "min",
 }

+INVALID_FREQ_ERR_MSG = "Invalid frequency: {0}"


Suggested change

INVALID_FREQ_ERR_MSG = "Invalid frequency: {0}"

cdef str INVALID_FREQ_ERR_MSG = "Invalid frequency: {0}"

I think CI failures are unrelated to my changes.

MarcoGorelli

thanks for your PR!

Is it necessary to introduce c_REMOVED_ABBREVS? I tried just removing it altogether, i.e.

diff --git a/pandas/_libs/tslibs/dtypes.pxd b/pandas/_libs/tslibs/dtypes.pxd
index 1ceb837d39..d8c536a34b 100644
--- a/pandas/_libs/tslibs/dtypes.pxd
+++ b/pandas/_libs/tslibs/dtypes.pxd
@@ -14,7 +14,6 @@ cdef bint is_supported_unit(NPY_DATETIMEUNIT reso)
 cdef dict c_OFFSET_TO_PERIOD_FREQSTR
 cdef dict c_PERIOD_TO_OFFSET_FREQSTR
 cdef dict c_OFFSET_RENAMED_FREQSTR
-cdef dict c_REMOVED_ABBREVS
 cdef dict c_DEPR_UNITS
 cdef dict c_PERIOD_AND_OFFSET_DEPR_FREQSTR
 cdef dict attrname_to_abbrevs
diff --git a/pandas/_libs/tslibs/dtypes.pyx b/pandas/_libs/tslibs/dtypes.pyx
index 08a398f7a3..f1cb5ce9fb 100644
--- a/pandas/_libs/tslibs/dtypes.pyx
+++ b/pandas/_libs/tslibs/dtypes.pyx
@@ -335,14 +335,6 @@ PERIOD_TO_OFFSET_FREQSTR = {
 cdef dict c_OFFSET_TO_PERIOD_FREQSTR = OFFSET_TO_PERIOD_FREQSTR
 cdef dict c_PERIOD_TO_OFFSET_FREQSTR = PERIOD_TO_OFFSET_FREQSTR
 
-# Map deprecated resolution abbreviations to correct resolution abbreviations
-cdef dict c_REMOVED_ABBREVS = {
-    "H": "h",
-    "BH": "bh",
-    "CBH": "cbh",
-    "S": "s",
-}
-
 cdef dict c_DEPR_UNITS = {
     "w": "W",
     "d": "D",
@@ -451,9 +443,7 @@ class Resolution(Enum):
         True
         """
         try:
-            if freq in c_REMOVED_ABBREVS:
 
-                raise ValueError(INVALID_FREQ_ERR_MSG.format(freq))
             attr_name = _abbrev_to_attrnames[freq]
         except KeyError:
             # For quarterly and yearly resolutions, we need to chop off
@@ -464,8 +454,6 @@ class Resolution(Enum):
             if split_freq[1] not in _month_names:
                 # i.e. we want e.g. "Q-DEC", not "Q-INVALID"
                 raise
-            if split_freq[0] in c_REMOVED_ABBREVS:
-                raise ValueError(INVALID_FREQ_ERR_MSG.format(split_freq[0]))
             attr_name = _abbrev_to_attrnames[split_freq[0]]
 
         return cls.from_attrname(attr_name)
diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index a30d738cc1..6e17f8f6ae 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -60,7 +60,6 @@ from pandas._libs.tslibs.dtypes cimport (
     c_OFFSET_TO_PERIOD_FREQSTR,
     c_PERIOD_AND_OFFSET_DEPR_FREQSTR,
     c_PERIOD_TO_OFFSET_FREQSTR,
-    c_REMOVED_ABBREVS,
     periods_per_day,
 )
 from pandas._libs.tslibs.nattype cimport (
@@ -4908,8 +4907,6 @@ cpdef to_offset(freq, bint is_period=False):
                 if not stride:
                     stride = 1
 
-                if prefix in c_REMOVED_ABBREVS:
-                    raise ValueError(INVALID_FREQ_ERR_MSG.format(prefix))
 
                 if prefix in {"D", "h", "min", "s", "ms", "us", "ns"}:
                     # For these prefixes, we have something like "3h" or

and it seems fine, did I miss something?

In

pandas/pandas/_libs/tslibs/dtypes.pyx

Lines 447 to 457 in 7d2980c

    
           >>> Resolution.get_reso_from_freqstr('h') 
        
           <Resolution.RESO_HR: 5> 
        
           >>> Resolution.get_reso_from_freqstr('h') == Resolution.RESO_HR 
        
           True 
        
           """ 
        
           try: 
        
               if freq in c_REMOVED_ABBREVS: 
        
                   raise ValueError(INVALID_FREQ_ERR_MSG.format(freq)) 
        
               attr_name = _abbrev_to_attrnames[freq]

you may also want to change raise to something like raise ValueError(err_msg) from exc, where exc is the KeyError?

…yError in get_reso_from_freqstr

This reverts commit 4085d5c.

…from_freqstr

natmokval · 2024-07-09T13:11:01Z

thanks for your PR!

Is it necessary to introduce c_REMOVED_ABBREVS? I tried just removing it altogether, i.e.

diff --git a/pandas/_libs/tslibs/dtypes.pxd b/pandas/_libs/tslibs/dtypes.pxd
index 1ceb837d39..d8c536a34b 100644
--- a/pandas/_libs/tslibs/dtypes.pxd
+++ b/pandas/_libs/tslibs/dtypes.pxd
@@ -14,7 +14,6 @@ cdef bint is_supported_unit(NPY_DATETIMEUNIT reso)
 cdef dict c_OFFSET_TO_PERIOD_FREQSTR
 cdef dict c_PERIOD_TO_OFFSET_FREQSTR
 cdef dict c_OFFSET_RENAMED_FREQSTR
-cdef dict c_REMOVED_ABBREVS
 cdef dict c_DEPR_UNITS
 cdef dict c_PERIOD_AND_OFFSET_DEPR_FREQSTR
 cdef dict attrname_to_abbrevs
diff --git a/pandas/_libs/tslibs/dtypes.pyx b/pandas/_libs/tslibs/dtypes.pyx
index 08a398f7a3..f1cb5ce9fb 100644
--- a/pandas/_libs/tslibs/dtypes.pyx
+++ b/pandas/_libs/tslibs/dtypes.pyx
@@ -335,14 +335,6 @@ PERIOD_TO_OFFSET_FREQSTR = {
 cdef dict c_OFFSET_TO_PERIOD_FREQSTR = OFFSET_TO_PERIOD_FREQSTR
 cdef dict c_PERIOD_TO_OFFSET_FREQSTR = PERIOD_TO_OFFSET_FREQSTR
 
-# Map deprecated resolution abbreviations to correct resolution abbreviations
-cdef dict c_REMOVED_ABBREVS = {
-    "H": "h",
-    "BH": "bh",
-    "CBH": "cbh",
-    "S": "s",
-}
-
 cdef dict c_DEPR_UNITS = {
     "w": "W",
     "d": "D",
@@ -451,9 +443,7 @@ class Resolution(Enum):
         True
         """
         try:
-            if freq in c_REMOVED_ABBREVS:
 
-                raise ValueError(INVALID_FREQ_ERR_MSG.format(freq))
             attr_name = _abbrev_to_attrnames[freq]
         except KeyError:
             # For quarterly and yearly resolutions, we need to chop off
@@ -464,8 +454,6 @@ class Resolution(Enum):
             if split_freq[1] not in _month_names:
                 # i.e. we want e.g. "Q-DEC", not "Q-INVALID"
                 raise
-            if split_freq[0] in c_REMOVED_ABBREVS:
-                raise ValueError(INVALID_FREQ_ERR_MSG.format(split_freq[0]))
             attr_name = _abbrev_to_attrnames[split_freq[0]]
 
         return cls.from_attrname(attr_name)
diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx
index a30d738cc1..6e17f8f6ae 100644
--- a/pandas/_libs/tslibs/offsets.pyx
+++ b/pandas/_libs/tslibs/offsets.pyx
@@ -60,7 +60,6 @@ from pandas._libs.tslibs.dtypes cimport (
     c_OFFSET_TO_PERIOD_FREQSTR,
     c_PERIOD_AND_OFFSET_DEPR_FREQSTR,
     c_PERIOD_TO_OFFSET_FREQSTR,
-    c_REMOVED_ABBREVS,
     periods_per_day,
 )
 from pandas._libs.tslibs.nattype cimport (
@@ -4908,8 +4907,6 @@ cpdef to_offset(freq, bint is_period=False):
                 if not stride:
                     stride = 1
 
-                if prefix in c_REMOVED_ABBREVS:
-                    raise ValueError(INVALID_FREQ_ERR_MSG.format(prefix))
 
                 if prefix in {"D", "h", "min", "s", "ms", "us", "ns"}:
                     # For these prefixes, we have something like "3h" or

and it seems fine, did I miss something?

thanks, @MarcoGorelli for helping me with this PR. You are right, there is no need to have c_REMOVED_ABBREVS. I removed it altogether.

natmokval · 2024-07-09T13:25:36Z

In

pandas/pandas/_libs/tslibs/dtypes.pyx

Lines 447 to 457 in 7d2980c

>>> Resolution.get_reso_from_freqstr('h')

<Resolution.RESO_HR: 5>

>>> Resolution.get_reso_from_freqstr('h') == Resolution.RESO_HR

True

"""

try:

if freq in c_REMOVED_ABBREVS:

raise ValueError(INVALID_FREQ_ERR_MSG.format(freq))

attr_name = _abbrev_to_attrnames[freq]

you may also want to change raise to something like raise ValueError(err_msg) from exc, where exc is the KeyError?

thanks, I did as you suggested and replaced raise with raise ValueError(msg) from exc, where exc is the KeyError, e.g.

except KeyError as exc:
    msg = INVALID_FREQ_ERR_MSG.format(freq)
    # For quarterly and yearly resolutions, we need to chop off
    #  a month string.
    split_freq = freq.split("-")
    if len(split_freq) != 2:
        raise ValueError(msg) from exc

MarcoGorelli

thanks @natmokval , looks good to me, nice to see things get simpler

mroeschke · 2024-07-09T17:19:37Z

Thanks @natmokval

natmokval added 3 commits June 28, 2024 22:18

CLN: enforced the deprecation of strings ‘H’, ‘BH’, ‘CBH’ in favour o…

3191ab4

…f ‘h’, ‘bh’, ‘cbh’

fix tests

432c604

add a note to v3.0.0

b4448bd

natmokval added the Clean label Jul 1, 2024

natmokval marked this pull request as ready for review July 1, 2024 18:38

natmokval requested a review from MarcoGorelli as a code owner July 1, 2024 18:38

mroeschke reviewed Jul 1, 2024

View reviewed changes

natmokval added 2 commits July 2, 2024 10:43

fixup

7d2980c

add def INVALID_FREQ_ERR_MSG to dtypes.pxd

4085d5c

MarcoGorelli reviewed Jul 4, 2024

View reviewed changes

natmokval added 3 commits July 9, 2024 13:09

resolve conflicts, remove dict c_REMOVED_ABBREVS, add msg if raise Ke…

44740c0

…yError in get_reso_from_freqstr

Revert "add def INVALID_FREQ_ERR_MSG to dtypes.pxd"

8d4ec2a

This reverts commit 4085d5c.

remove dict c_REMOVED_ABBREVS, add msg if raise KeyError in get_reso_…

412ab91

…from_freqstr

natmokval force-pushed the cln-enforce-deprec-H-BH-CBH branch from 4a3e0de to 412ab91 Compare July 9, 2024 11:57

MarcoGorelli approved these changes Jul 9, 2024

View reviewed changes

mroeschke approved these changes Jul 9, 2024

View reviewed changes

mroeschke added this to the 3.0 milestone Jul 9, 2024

mroeschke merged commit d966462 into pandas-dev:main Jul 9, 2024
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: enforced the deprecation of strings 'H', 'BH', 'CBH' in favor of 'h', 'bh', 'cbh' #59143

CLN: enforced the deprecation of strings 'H', 'BH', 'CBH' in favor of 'h', 'bh', 'cbh' #59143

natmokval commented Jun 28, 2024

mroeschke Jul 1, 2024

natmokval Jul 2, 2024

mroeschke Jul 1, 2024

natmokval Jul 2, 2024

natmokval Jul 2, 2024

MarcoGorelli left a comment

natmokval commented Jul 9, 2024

natmokval commented Jul 9, 2024

MarcoGorelli left a comment

mroeschke commented Jul 9, 2024

	def test_CBH_deprecated(self, freq):
	def test_CBH_raises(self, freq):

	INVALID_FREQ_ERR_MSG = "Invalid frequency: {0}"
	cdef str INVALID_FREQ_ERR_MSG = "Invalid frequency: {0}"

	>>> Resolution.get_reso_from_freqstr('h')
	<Resolution.RESO_HR: 5>

	>>> Resolution.get_reso_from_freqstr('h') == Resolution.RESO_HR
	True
	"""
	try:
	if freq in c_REMOVED_ABBREVS:

	raise ValueError(INVALID_FREQ_ERR_MSG.format(freq))
	attr_name = _abbrev_to_attrnames[freq]

CLN: enforced the deprecation of strings 'H', 'BH', 'CBH' in favor of 'h', 'bh', 'cbh' #59143

CLN: enforced the deprecation of strings 'H', 'BH', 'CBH' in favor of 'h', 'bh', 'cbh' #59143

Conversation

natmokval commented Jun 28, 2024

mroeschke Jul 1, 2024

Choose a reason for hiding this comment

natmokval Jul 2, 2024

Choose a reason for hiding this comment

mroeschke Jul 1, 2024

Choose a reason for hiding this comment

natmokval Jul 2, 2024

Choose a reason for hiding this comment

natmokval Jul 2, 2024

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

natmokval commented Jul 9, 2024

natmokval commented Jul 9, 2024

MarcoGorelli left a comment

Choose a reason for hiding this comment

mroeschke commented Jul 9, 2024