Skip to content

Conversation

@natmokval
Copy link
Contributor

@natmokval natmokval commented Oct 26, 2025

we deprecated the frequency "CBH" in favor of "cbh". Now bdate_range raises for both "CBH" and "cbh". Corrected the bdate_range to work on "cbh".

@natmokval natmokval added the Frequency DateOffsets label Oct 26, 2025
raise TypeError(msg)

if isinstance(freq, str) and freq.startswith("C"):
if isinstance(freq, str) and freq.upper().startswith("C"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to allow strings that start with uppercase "C"? or just lowercase?

Copy link
Contributor Author

@natmokval natmokval Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbrockmendel, thanks for reviewing this PR.
I think we want to cover both cases, strings start with "C" and with "c", because bdate_range should work for all custom frequencies.

I would prefer to do an explicit check on line 1136, like freq in ["CBMS", "CBME", "C", "cbh"]. In this case, there’s no reason to keep the exception block below.

except (KeyError, TypeError) as err:
msg = f"invalid custom frequency string: {freq}"
raise ValueError(msg) from err

We should also update the error message in the test_all_custom_freq.
Do you think, it is better to check explicit insted of freq.upper().startswith("C")?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess if they pass an upper-case the ideal thing would be an exception message telling them specifically to do lower case? im happy to defer to your judgement on this

Copy link
Contributor Author

@natmokval natmokval Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess if they pass an upper-case the ideal thing would be an exception message telling them specifically to do lower case? im happy to defer to your judgement on this

I agree, for incorrect casing we could raise an error message and suggest correct casing. It seems we only have one incorrect casing for custom frequencies: "CBH". Other cases like "cbme", "c" or "Cbh" we consider as invalid frequencies.

I will make changes and add the specific exception message with suggestion to do lower case for "CBH".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Frequency DateOffsets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: bdate_range with cbh fails

2 participants