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

Fix Ticker.history bugs #129

Merged
merged 3 commits into from
Dec 28, 2022
Merged

Fix Ticker.history bugs #129

merged 3 commits into from
Dec 28, 2022

Conversation

maread99
Copy link
Contributor

Fixes:

See the referenced issues for the background.

Also, gets upwards of 2x reduction in data processing time by reducing DataFrame manipulations.

@dpguthrie, give this a 👍 if you like the look of it and I'll add some tests! (Changes are passing the existing test suite locally.)

Fixes:
- dpguthrie#127 BUG: inaccurate evaluation of 'day' corresponding with close
price.
- dpguthrie#128 BUG: inaccurate intraday price indexing when exchanges
observe DST.

Also, improves performance by reducing DataFrame manipulations.
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Base: 94.60% // Head: 91.95% // Decreases project coverage by -2.64% ⚠️

Coverage data is based on head (9d8fd98) compared to base (7ab4f4f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   94.60%   91.95%   -2.65%     
==========================================
  Files          10       10              
  Lines         853      883      +30     
==========================================
+ Hits          807      812       +5     
- Misses         46       71      +25     
Impacted Files Coverage Δ
yahooquery/ticker.py 96.10% <100.00%> (+0.01%) ⬆️
yahooquery/utils/__init__.py 97.36% <100.00%> (+0.89%) ⬆️
yahooquery/login.py 31.25% <0.00%> (-37.50%) ⬇️
yahooquery/base.py 93.00% <0.00%> (-3.50%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maread99
Copy link
Contributor Author

maread99 commented Nov 23, 2022

WRT the timezone of the returned index, at the moment this PR maintains the existing behavior, which is to say that the index of intraday data and the 'live indice' of daily data are returned with no timezone information. The actual timezone that the values reflect will be utc or local as determined by the adj_timezone parameter. @dpguthrie, would you prefer these objects to include the timezone info? Personally I'd favor including it in the interest making clear to the user what they've been served. (EDIT - I added the timezone info - I can take it back out if you would prefer.)

Adds timezone info to:
- index of intraday data.
- any live indice of daily data.

Also, sets column order of return as ohlcv then any 'adjclose',
'dividends', 'splits'.
@maread99 maread99 force-pushed the fix-history branch 3 times, most recently from 231190f to b392d67 Compare November 25, 2022 00:20
@maread99
Copy link
Contributor Author

Added tests.

I've also set the order of the columns of the returned dataframe to ohlcv followed by any 'adjclose', 'dividends', 'splits' (previously, the order of the ohlcv columns would be seemingly random).

The failing project coverage check appears unrelated (in the login.py module, possibly related to #130, perhaps a flaky test that is aborting on not getting a connection?).

Anyway, PR's ready for review.

Cheers

Adds `TestHistoryDataframe` test class.
@dpguthrie
Copy link
Owner

Thanks for opening this @maread99 ! Appreciate all the work here! I'll have a bit more time to look into this later this week.

@maread99
Copy link
Contributor Author

Hi @dpguthrie, I know time is always an issue, although I was hoping you could give an indication as to when you think you might be in a position to look at this.
Cheers

@dpguthrie
Copy link
Owner

@maread99 Apologize again for the delay here! This is 🔥 ! Thank you for contributing this; it will really help with others in really adopting this package.

@dpguthrie dpguthrie merged commit 4b4c2c1 into dpguthrie:master Dec 28, 2022
@maread99
Copy link
Contributor Author

No worries @dpguthrie, thanks for merging.

Given all your recent changes and this PR, will you be cutting a new release for PyPI any time soon?

@maread99 maread99 deleted the fix-history branch December 28, 2022 22:39
@maread99 maread99 mentioned this pull request Jan 9, 2023
maread99 added a commit to maread99/yahooquery that referenced this pull request Feb 5, 2023
Changes dtype of index of DataFrame returned by `Ticker.history` when
interval is in terms of weeks or months. Changes dtype from tz-aware
'datetime64' to 'object' that represents dates and live indice in the
same manner as for a daily interval.

This behaviour should have been implemented in dpguthrie#129.
@maread99 maread99 mentioned this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants