Skip to content

Conversation

@snitish
Copy link
Member

@snitish snitish commented Feb 17, 2025

@snitish snitish marked this pull request as draft February 17, 2025 02:28
@snitish
Copy link
Member Author

snitish commented Feb 17, 2025

@rhshadrach I was able to add the half-year offset classes without too much added complexity. However, I haven't enabled them to be used as periods - that will take more code changes and tests. Do you recommend concluding this here or go all the way to supporting half-year periods? cc: @MarcoGorelli

Copy link

@rwijtvliet rwijtvliet left a comment

Choose a reason for hiding this comment

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

Looks great sofar to me

@rhshadrach
Copy link
Member

@snitish - I think holding off on periods makes sense. This is already a sizable PR.

@snitish snitish marked this pull request as ready for review February 22, 2025 19:09
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines 3021 to 3023
# FIXME(cython#4446): python annotation here gives compile-time errors
# _default_starting_month: int
# _from_name_starting_month: int
Copy link
Member

Choose a reason for hiding this comment

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

Just curious if you've tried seeing if this is resolved.

cython/cython#4446 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, this is not resolved yet. Still throws compile time errors when I uncomment those lines -

 pandas/_libs/tslibs/offsets.cpython-310-darwin.so.p/pandas/_libs/tslibs/offsets.pyx.c:98384:21: error: use of undeclared identifier '__pyx_base' 98384 | __Pyx_XDECREF_SET(__pyx_base._default_starting_month, ((PyObject*)__pyx_t_2)); | ^ pandas/_libs/tslibs/offsets.cpython-310-darwin.so.p/pandas/_libs/tslibs/offsets.pyx.c:98384:21: error: use of undeclared identifier '__pyx_base'; did you mean '__pyx_k_base'? 98384 | __Pyx_XDECREF_SET(__pyx_base._default_starting_month, ((PyObject*)__pyx_t_2)); | ^~~~~~~~~~ | __pyx_k_base 
Copy link
Member

Choose a reason for hiding this comment

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

I think the resolution would be type-hinting as:

_default_starting_month: typing.ClassVar[int] 

That's what I'm wondering will work with Cython 3.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, that seems to have fixed it. Thanks!

@mroeschke mroeschke added this to the 3.0 milestone Mar 3, 2025
@mroeschke mroeschke added the Frequency DateOffsets label Mar 3, 2025
@mroeschke mroeschke merged commit 826f0d3 into pandas-dev:main Mar 3, 2025
46 checks passed
@mroeschke
Copy link
Member

Thanks again @snitish

tonyyuyiding pushed a commit to tonyyuyiding/pandas that referenced this pull request Mar 4, 2025
* ENH: Add HalfYear offsets * Add entry to whatsnew * Resolve cython typing issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Frequency DateOffsets

4 participants