Skip to content

BUG: Breaking change to offsets.pyx - MonthOffset #39887

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

Closed
wants to merge 71 commits into from

Conversation

Pawel-Kranzberg
Copy link
Contributor

@Pawel-Kranzberg Pawel-Kranzberg commented Feb 18, 2021

Add ability to use class MonthBegin as a period.
It would address #38914 and #38859.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can you add a test for this that fails on current master.

you also need to adjust tests for this change.

@jreback jreback added the Frequency DateOffsets label Feb 19, 2021
@jbrockmendel
Copy link
Member

im wary of this. inevitably someone is going to be confused as to why Period(foo, freq= BusinessMonthEnd) isnt different from Period(foo, freq= BusinessMonthBegin)

If we were doing this from scratch, I would keep DateOffsets totally unrelated to PeriodDtypes.

@Pawel-Kranzberg Pawel-Kranzberg marked this pull request as draft March 30, 2021 12:24
@Pawel-Kranzberg Pawel-Kranzberg marked this pull request as ready for review April 23, 2021 07:48
@Pawel-Kranzberg Pawel-Kranzberg changed the title [BUG] Breaking change to offsets.pyx - MonthOffset BUG: Breaking change to offsets.pyx - MonthOffset Apr 23, 2021
@Pawel-Kranzberg
Copy link
Contributor Author

can you add a test for this that fails on current master.

you also need to adjust tests for this change.

done

@jbrockmendel
Copy link
Member

I find this really sketchy. MonthBegin doesn't correspond to a Period. I just dont understand the motivation here.

@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label May 29, 2021
@Pawel-Kranzberg Pawel-Kranzberg marked this pull request as draft June 10, 2021 17:03
@simonjayhawkins
Copy link
Member

@Pawel-Kranzberg can you address #39887 (comment)

@Pawel-Kranzberg
Copy link
Contributor Author

Pawel-Kranzberg commented Jun 17, 2021

@Pawel-Kranzberg can you address #39887 (comment)

I plan to do it next week. In a nutshell, there are existing issues that reference the need for MonthBegin as Offset and I also faced it during a project at work. Ended up coding a workaround that made code maintenance more challenging.

@mroeschke
Copy link
Member

Thanks for the PR, but it appears that it has gotten stale and there's some uncertainty that around this proposed change.

Closing, but glad to have you help on other open issues.

@mroeschke mroeschke closed this Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Timestamp.to_period() fails for freq="MS"
5 participants