Skip to content

CLN: remove unused entries from _lite_rule_alias #56516

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

CLN: remove unused entries from _lite_rule_alias #56516

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b149d25
remove redundant entries from _lite_rule_alias
natmokval Dec 14, 2023
d6e259c
Merge branch 'main' into enh-remove_abbrevs--lite-rule-alias
natmokval Dec 14, 2023
df68115
fix test
natmokval Dec 15, 2023
ed3e8f5
resolve conflicts
natmokval Jul 17, 2024
e518042
add US, NS to c_PERIOD_AND_OFFSET_DEPR_FREQSTR, fix tests
natmokval Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions pandas/_libs/tslibs/offsets.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4644,10 +4644,6 @@ _lite_rule_alias = {
"BYS": "BYS-JAN", # BYearBegin(month=1),

"Min": "min",
"min": "min",
"ms": "ms",
"us": "us",
"ns": "ns",
}

_dont_uppercase = {
Expand All @@ -4672,6 +4668,9 @@ _dont_uppercase = {
"qe-oct",
"qe-nov",
"ye",
"min",
"ns",
"us",
Copy link
Member

Choose a reason for hiding this comment

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

can you remove unrelated changes please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we remove these from _dont_uppercase the tests test_offset_freqstr() and test_rule_code() in pandas/tests/tseries/offsets/test_offsets.py will fail. The reason: we uppercase names of offsets class in _get_offset(). In PR #56346 where we deprecate uppercasing/lowercasing we will remove them from _dont_uppercase

Copy link
Member

Choose a reason for hiding this comment

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

ah, thanks - so, they're not currently unused then? but will become unused once #56346 is enforced? should this PR wait until after that one then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked in PR #56346, we should keep "min", "us", "ns" in the list _dont_uppercase to pass these tests.
On the other hand, we have all lowercase offsets frequencies in this list ('h', 's', etc.), and it looks more clear then have these entries ("min": "min", "us": "us", etc.) in dict _lite_rule_alias.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for explaining

sorry for not having got round to this yet, was quite this week with a client project - but I'll get to it, thanks for looking at it all so carefully!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for helping me with my PRs!

}


Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/indexes/datetimes/test_partial_slicing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ def test_partial_slice_second_precision(self):
rng = date_range(
start=datetime(2005, 1, 1, 0, 0, 59, microsecond=999990),
periods=20,
freq="US",
freq="us",
Copy link
Member

Choose a reason for hiding this comment

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

does this PR make to_offset('US') suddently start failing? if so, I don't think we can do this yet (at least, not before https://github.com/pandas-dev/pandas/pull/56346/files ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this PR make to_offset('US') suddently start failing? if so, I don't think we can do this yet (at least, not before https://github.com/pandas-dev/pandas/pull/56346/files ?)

yes, this test didn't fail for uppercase frequency "US" before this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a small correction to the comment above:
we fixed the test_partial_slice_second_precision in PR DEPR: lowercase strings denoting freq for Week, MonthBegin, MonthEnd, etc. #56346
but so far on main we have to_offset('US') in this test and it pass.

)
s = Series(np.arange(20), rng)

Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/scalar/period/test_period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def test_construction(self):
assert i1 == i3

i1 = Period("1982", freq="min")
i2 = Period("1982", freq="MIN")
i2 = Period("1982", freq="Min")
assert i1 == i2

i1 = Period(year=2005, month=3, day=1, freq="D")
Expand Down