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

One Minute History Groundwork #340

Closed
wants to merge 6 commits into from
Closed

One Minute History Groundwork #340

wants to merge 6 commits into from

Conversation

@ssanderson
Copy link
Contributor

@ssanderson ssanderson commented May 30, 2014

Overhauls HistoryContainer in prep for support of more than one frequency.

Major changes:

  • Methods/variables referring to "day" have been renamed/generalized.
    • current_day_panel became buffer_panel, which is now a RollingPanel
    • prior_day_panel became a dictionary mapping Frequency objects to
      "digest panels", which are instances of RollingPanel.
  • Hard-coded daily rollover replaced with a notion of a "current window" for
    each unique frequency managed by the panel.
    • When the end of the current window is reached for a given frequency, we
      compute an aggregate bar (code refers to this as a "digest"), which is
      appended to a panel associated with that frequency.
    • Window rollover dates are managed by a pair of dictionaries,
      cur_window_starts and cur_window_closes. The Frequency class is
      responsible for computing window bounds based on the open/close of the
      previous window.
  • Semantic change to the open_price field: open_price now always
    contains the price of the first trade occurring in the given window.
    Previously it contained the price of the first minute in the window,
    returning NaN it the security happened not to trade in the first minute.
ssanderson added 4 commits May 16, 2014
Adds a suite of new functions for querying data from the trading calendar.

These include:
      `previous_trading_day`
      `minutes_for_days_in_range` (minutely version of `days_in_range`)
      `previous_open_and_close` (inverse of `next_open_and_close`)
      `next_market_minute`
      `previous_market_minute`
      `open_close_window` (get a range of opens/closes with slicing semantics)
      `market_minute_window` (get a range of minutes with slicing semantics)

Also refactors `test_finance` to move `TradingEnvironment` tests into their own
TestCase.
Overhauls `HistoryContainer` in prep for support of more than one frequency.

Major changes:

   - Methods/variables referring to "day" have been renamed/generalized.
     - `current_day_panel` became `buffer_panel`, which is now a `RollingPanel`
     - `prior_day_panel` became a dictionary mapping `Frequency` objects to
       "digest panels", which are instances of `RollingPanel`.

   - Hard-coded daily rollover replaced with a notion of a "current window" for
     each unique frequency managed by the panel.

     - When the end of the current window is reached for a given frequency, we
       compute an aggregate bar (code refers to this as a "digest"), which is
       appended to a panel associated with that frequency.

     - Window rollover dates are managed by a pair of dictionaries,
       `cur_window_starts` and `cur_window_closes`.  The `Frequency` class is
       responsible for computing window bounds based on the open/close of the
       previous window.

   - Semantic change to the `open_price` field: `open_price` now always
     contains the price of the first trade occurring in the given window.
     Previously it contained the price of the first minute in the window,
     returning NaN it the security happened not to trade in the first minute.
@ssanderson ssanderson changed the title One minute history One Minute History Groundwork May 31, 2014
sim_params=factory.create_simulation_parameters()
)
sim_params=factory.create_simulation_parameters(),
)

This comment has been minimized.

@ehebert

ehebert Jun 2, 2014
Contributor

@ssanderson I think you need to update to the latest version of pyflakes and pep8.

see: 2debde3

This comment has been minimized.

@ssanderson

ssanderson Jun 2, 2014
Author Contributor

Hrm...I explicitly fixed these because they were causing Travis to fail. Is Travis updated to the latest?

This comment has been minimized.

@ssanderson

ssanderson Jun 2, 2014
Author Contributor

Further confusion: on the latest pep8/pyflakes, I get no error for this when I run
flake8 tests/test_exception_handling.py
but, if I explicitly pass flake8 tests/test_exception_handling.py --select=E
then I see

tests/test_exception_handling.py:85:13: E123 closing bracket does not match indentation of opening bracket's line
tests/test_exception_handling.py:105:13: E123 closing bracket does not match indentation of opening bracket's line
tests/test_exception_handling.py:125:13: E123 closing bracket does not match indentation of opening bracket's line

This comment has been minimized.

@ehebert

ehebert Jun 2, 2014
Contributor

The Travis config should be in sync via grep-ing out of requirements_dev.txt

Not sure why you are getting different results from different invocations.

This comment has been minimized.

@ssanderson

ssanderson Jun 2, 2014
Author Contributor

FWIW, the Travis build appears to be passing with this as it's written.

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/history/history_container.py in c7d7c1f Jun 2, 2014

FWIW, this purposely had the two different statements with a line break between.

But not going to quibble too much over it.

This comment has been minimized.

Copy link
Contributor Author

@ssanderson ssanderson replied Jun 2, 2014

The actual meaningful change here was algoproxy -> TradingAlgorithm. The reformatting probably just happened because I hit M-q on the region.

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert replied Jun 2, 2014

(This comment mostly applies for Emacs users.)

Hmm, maybe we should think about having a .dir-locals.el, for consistency of 'sentence-end-double-space across contributors.

Whether that value is 'nil or 't. I can be talked into either way, but my preference is for 'nil

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/finance/trading.py in a98b690 Jun 2, 2014

Could you add a note about how it is incorrectly handled here?

It looks like the problem is that near the end of the data the index + offset will be out of bounds, correct?

This comment has been minimized.

Copy link
Contributor Author

@ssanderson ssanderson replied Jun 2, 2014

Yeah, there's other places where we throw a NoFurtherDataError, whereas this will probably result in a KeyError or an IndexError right now.

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/finance/trading.py in a98b690 Jun 2, 2014

I can't recall if we tried to, and dropped it because of an issue with timezones, but we might be able to use the tradingcalendar.trading_day for this and the next_trading_day function instead.

Something like (assuming trading_day is set in the __init__):

def previous_trading_day(self, test_date):
    dt = self.normalize_date(test_date)
    try:
        return dt - self.trading_day
    except WhateverExceptionIsRaisedWhenIndexExceeded:
        return None

Wouldn't call that a blocking change here, but worth investigating.

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/history/history.py in 269fdff Jun 2, 2014

Not blocking, but I'd recommend "{0}({1})".format(self.__class__.__name__, freq_str)

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/history/history.py in 269fdff Jun 2, 2014

Let's drop these commented out lines.

This comment has been minimized.

Copy link
Contributor Author

@ssanderson ssanderson replied Jun 2, 2014

Oops, yep, these are just an implementation artifact of a different idea I had for calculating window bounds.

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/history/history_container.py in 269fdff Jun 2, 2014

Should we just have the __hash__ of HistorySpec return the tuple instead?
So that we don't need to pass a key to the sorted call?

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert replied Jun 2, 2014

Nevermind, forgot the HistorySpec has the field as well.

@ehebert

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert commented on zipline/finance/trading.py in a98b690 Jun 2, 2014

Let's comment this with the performance gains you found by using arange instead of native slicing.

@ehebert
Copy link
Contributor

@ehebert ehebert commented Jun 2, 2014

Looks good. /signoff

Have you profiled this versus the current implementation with a 1d history algorithm?
If so, and it's comparable (or better!), ping me when you are done with edits on this branch, and I'll do the merge.
(Going to do some edits on master for the comments etc.)

@ssanderson

This comment has been minimized.

Copy link
Contributor Author

@ssanderson ssanderson commented on zipline/history/history_container.py in 269fdff Jun 2, 2014

Just found a bug here: we're only using the historical data if ffill = True. In related news, we don't have any test coverage for ffill=False.

This comment has been minimized.

Copy link
Contributor

@ehebert ehebert replied Jun 3, 2014

Good catch. Mind fixing, or opening up a separate issue for it?

ssanderson added 2 commits Jun 3, 2014
Fixes an issue where, if `ffill=False`, `get_history` would return nans for
every entry in the history frame except the last one.
@ssanderson
Copy link
Contributor Author

@ssanderson ssanderson commented Jun 4, 2014

Closing this in favor of updated PR.

@ssanderson ssanderson closed this Jun 4, 2014
@freddiev4 freddiev4 deleted the one_minute_history branch Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.