BUG: Fix parsing of stata dates (#17797) by miker985 · Pull Request #17990 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes StataReader selectively ignores format details #17797
- tests added / passed
- passes
git diff upstream/master -u -- "*.py" | flake8 --diff - Test and expand parsing of STATA date/time objects.
Expands behavior provided by the following to include most STATA format codes.
# remove format details from %td
self.fmtlist = ["%td" if x.startswith("%td") else x
for x in self.fmtlist]Add tests for above behavior (previously untested) + all additional format codes
| STATA supports 9 date types which each have distinct units. We test 7 |
|---|
| of the 9 types, ignoring %tC and %tb. %tC is a variant of %tc that |
| accounts for leap seconds and %tb relies on STATAs business calendar. |
| """ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great description! Can you convert that to a comment block? That's more aligned with how we comment our tests, and also, reference the issue number at the top (instead of at the bottom).
@miker985 : You're going to need to add a whatsnew for this PR. However, as the doc for 0.21.1 is not available yet, just hold tight. Feel free to ping us later if we forget to let you know.
@gfyoung Is this the comment you're looking for?
I'll wait to hear back on the whatsnew file. If I don't hear back in e.g., a week should I ping you?
| 'column', ['ms', 'day', 'week', 'month', 'qtr', 'half', 'yr']) |
|---|
| def test_date_parsing_ignores_format_details(self, column): |
| # GH 17797 |
| # Test that display formats are ignored when determining if a numeric |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: add a newline under the issue reference.
If I don't hear back in e.g., a week should I ping you?
@TomAugspurger should be merging the PR for the whatsnew soon, so that works.
| df = read_stata(self.stata_dates) |
|---|
| unformatted = df.loc[0, column] |
| formatted = df.loc[0, column + "_fmt"] |
| assert unformatted == formatted |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these supposed to be datetime64[ns] dtype?
what happens for the ignored formats? should raise?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these supposed to be
datetime64[ns]dtype?
At this point in the code formatted and unformatted are pandas._libs.tslib.Timestamp objects. Every column in df has a dtype of datetime64[ns]
what happens for the ignored formats? should raise?
Ignored formats are not converted to dates (consistent with previous behavior) source
can you add a whatsnew for 0.21.1 (bug fix io section)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment. ping on green.
| @@ -74,6 +74,9 @@ Indexing |
|---|
| I/O |
| ^^^ |
| - Bug in `StataReader` not converting date/time columns with display formatting addressed (:issue:`17990`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do class:`~pandas.io.stata.StataReader`. Can you make a bit more clear what is being fixed here.
peterpanmj pushed a commit to peterpanmj/pandas that referenced this pull request
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request
TomAugspurger pushed a commit to TomAugspurger/pandas that referenced this pull request
(cherry picked from commit e886af5)