Skip to content
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

correction to get time interpolation correct #39886

Closed
wants to merge 3 commits into from

Conversation

oricou
Copy link

@oricou oricou commented Feb 18, 2021

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.

you need to either add a test or change current ones to show what you are changing.

@jreback jreback added the Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate label Feb 19, 2021
@pep8speaks
Copy link

pep8speaks commented Feb 19, 2021

Hello @oricou! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-19 12:03:39 UTC

@oricou
Copy link
Author

oricou commented Feb 22, 2021

you need to either add a test or change current ones to show what you are changing.

Done but the "Missing-data" is still here...

@@ -86,6 +86,8 @@
Tick,
)

from pandas.core.reshape.concat import concat
Copy link
Contributor

Choose a reason for hiding this comment

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

your imports are in the wrong order and causing failed checks.

run > isort pandas

obj = self._selected_obj
tmp = concat([obj, result]).sort_index().interpolate(method='time')
tmp = tmp[result.index]
result[...] = tmp[~tmp.index.duplicated(keep='first')]
Copy link
Contributor

Choose a reason for hiding this comment

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

you are not following standard code guideines, e.g. use " instead of '.

run > black pandas this will automatically fix these issues for you.

@@ -1816,3 +1816,14 @@ def test_resample_aggregate_functions_min_count(func):
index=DatetimeIndex(["2020-03-31"], dtype="datetime64[ns]", freq="Q-DEC"),
)
tm.assert_series_equal(result, expected)


def timeseries_interpolation():
Copy link
Contributor

Choose a reason for hiding this comment

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

this will not run as a test. pytest will run all files of the form test_*.py or *_test.py

rename this test_timeseries_interpolation

Copy link
Author

Choose a reason for hiding this comment

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

I think I have done 90 % of the job, now I let maintainers finishing it. It should take them minutes while it would take my hours to polish the PR.

This bug has been reported 3 years ago and nobody from Pandas tried anything while a solution was proposed in the bug report. Now, 3 years after, I have done a PR. I know it is not perfect but it must be very close to what it should be. I hope it will not take 3 more years.

@attack68 It is not against you. I don't want to become a maintainers but just to help at my level.

Copy link
Author

Choose a reason for hiding this comment

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

@attack68 or any one else: feel free to for from my work and to finish the PR.

Copy link
Member

Choose a reason for hiding this comment

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

@oricou the main thing you need to do at this point is rename this function as @attack68 suggested.

I think I have done 90 % of the job

there are 58k lines of code in pandas/core/ and 220k in pandas/tests/. proper testing is important.

@jreback
Copy link
Contributor

jreback commented Feb 23, 2021

@oricou open source software is contributed by the community

the maintainers are donating time to review and comment and once the code meets all criteria it be merged

@mroeschke
Copy link
Member

Thanks @oricou for taking the time to address this issue with the PR, but since this PR has gotten stale I'm going to close for now. If anyone else is interested in taking this up, happy to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time Series Interpolation is wrong
6 participants