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

Update test_algorithm.py to use WithMakeAlgo. #2171

Merged
merged 14 commits into from May 8, 2018

Conversation

Projects
None yet
4 participants
@ssanderson
Member

ssanderson commented May 3, 2018

Updated about half of test_algorithm.py to use WithMakeAlgo.

Most of these changes are straightforward no-op changes. A notable exception is TestTransformAlgorithm, which had evolved organically enough over the years that I decided to just scrap it and re-write the useful tests from the suite in a modern style. Those new tests live in test_ordering.py

@ssanderson ssanderson force-pushed the once-moar-with-fixtures branch from e01ce26 to 2467856 May 4, 2018

def test_record_incr(self):
algo = RecordAlgorithm(sim_params=self.sim_params, env=self.env)
output = algo.run(self.data_portal)

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

A general theme of this PR is that I tried to "inline" algorithms into test sites if they were small and if there were assertions in the test body that depended on the behavior of the algorithm.

For tests that were just "run algorithm X", I didn't see as much value in doing the inlining.

This comment has been minimized.

@ehebert

ehebert May 4, 2018

Member

Thank you. I agree with the inlining. While debugging (or just reading tests), this will reduce cognitive load.

START_DATE = pd.Timestamp('2006-01-03', tz='UTC')
END_DATE = pd.Timestamp('2006-01-04', tz='UTC')
SIM_PARAMS_DATA_FREQUENCY = 'minute'
sids = 1, 2
# FIXME: Pass a benchmark source instead of this.

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

The default behavior of WithMakeAlgo is to use the first equity sid as a benchmark, but that often doesn't work, because if you run an algorithm from START_DATE to END_DATE, then you don't have enough days of historical data to calculate benchmark returns (in particular, you need the close from the day before START_DATE to compute a daily benchmark return for the first day of the backtest.

Setting BENCHMARK_SID to None causes us to use benchmark returns retrieved from the TradingEnvironment, which uses a static set of data that's checked into git. I'm not sure what we want long-term for benchmarks in testing, but this is what we were already doing in all of these tests, and I didn't want to layer in more changes than were necessary. Once we have all these suites creating algorithms in a uniform way, it will be easier to make broad changes to how we handle benchmarks.

class TestTransformAlgorithm(WithLogger,

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

TestTransformAlgorithm was kind of a mess. Several of the tests in this suite turned out to be complicated no-ops (e.g., the tests for ordering futures ran an algorithm that only ordered equities). Also, none of these tests were about the old transform API, which is what the name of this suite originally referenced. It ultimately seemed easier and clearer to just break this up and re-write the tests worth keeping. Most of the re-written tests now live in test_ordering.py.

)
self.assertEqual(algo.sim_params.data_frequency, 'minute')
def test_order_rounding(self):

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

I moved this into its own suite.

sim_params=self.sim_params,
env=self.env)
daily_stats = algo.run(self.data_portal)
def test_portfolio_exited_position(self):

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

This is another algorithm that was inlined.

@@ -1284,13 +1038,13 @@ def make_equity_minute_bar_data(cls):
yield sid, create_minute_df_for_asset(
cls.trading_calendar,
cls.data_start,
cls.sim_params.end_session,
cls.END_DATE,

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

Many of the make_*_bar_data methods used sim_params to construct data, but since there's no ordering dependency between WithSimParams and With*BarReader, cls.sim_params isn't guaranteed to exist when this method is called. The right thing to do here is usually to use the calendar, which is a direct dependency of With*BarReader, and is therefore guaranteed to be initialized by the time this method is called.

This comment has been minimized.

@ehebert

ehebert May 4, 2018

Member

Thanks for cleaning this one up.

)
cal = cls.trading_calendars[Equity]
sessions = cal.sessions_in_range(cls.START_DATE, cls.END_DATE)
frame = pd.DataFrame({

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

The original impetus for rewriting this method was removing the dependency on cls.sim_params (see note above), but I think this also makes it much clearer what the price data is in this suite.

cls.data_portal = FakeDataPortal(cls.env)
# XXX: This suite doesn't use the data in its DataPortal; it uses a
# FakeDataPortal with different mock data.
def init_instance_fixtures(self):

This comment has been minimized.

@ssanderson

ssanderson May 4, 2018

Member

This needs to be in init_instance_fixtures because WithDataPortal sets self.data_portal as a instance attribute, which takes precedence over a class-level attribute.

This comment has been minimized.

@ehebert

ehebert May 4, 2018

Member

In another pass, we may want to convert these to use and expect data from the data_portal created by WithDataPortal. IIRC, FakeDataPortal was originally a shim that was introduced during development, before we had the full WithDataPortal fixture.

@coveralls

This comment has been minimized.

coveralls commented May 4, 2018

Coverage Status

Coverage decreased (-0.5%) to 86.734% when pulling 2467856 on once-moar-with-fixtures into 4d155ca on master.

@ehebert ehebert self-requested a review May 4, 2018

@ehebert

ehebert approved these changes May 4, 2018

Thanks for bringing more standardization to the test setup.

Let me know if it would useful to remove FakeDataPortal.

@@ -788,55 +759,67 @@ def test_future_symbol(self):
with self.assertRaises(TypeError):
algo.future_symbol({'foo': 'bar'})
class TestSetSymbolLookupDate(zf.WithMakeAlgo, zf.ZiplineTestCase):
# January 2006

This comment has been minimized.

@ehebert

ehebert May 4, 2018

Member

Nice choice of a month, with the alignment of the days of the week.

Also 👍 on removing the use of timedelta in constructing dates.

cls.data_portal = FakeDataPortal(cls.env)
# XXX: This suite doesn't use the data in its DataPortal; it uses a
# FakeDataPortal with different mock data.
def init_instance_fixtures(self):

This comment has been minimized.

@ehebert

ehebert May 4, 2018

Member

In another pass, we may want to convert these to use and expect data from the data_portal created by WithDataPortal. IIRC, FakeDataPortal was originally a shim that was introduced during development, before we had the full WithDataPortal fixture.

@ahgnaw

This comment has been minimized.

Contributor

ahgnaw commented May 4, 2018

I prodded around for a bit and lgtm

@ssanderson ssanderson merged commit e3f328d into master May 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ssanderson ssanderson deleted the once-moar-with-fixtures branch May 8, 2018

@ssanderson

This comment has been minimized.

Member

ssanderson commented May 8, 2018

@ehebert I think it probably makes sense to remove FakeDataPortal, but I don't have time to do that before pycon and I don't want this to go stale while I'm gone.

@ehebert

This comment has been minimized.

Member

ehebert commented May 8, 2018

@ehebert I think it probably makes sense to remove FakeDataPortal, but I don't have time to do that before pycon and I don't want this to go stale while I'm gone.

I agree with merging this without removing FakeDataPortal. I may have not been clear, I was offering to do the removal as a follow up to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment