Fix #385 among other things #395
Conversation
If datareader doesn't work, trying older version https://pandas.pydata.org/pandas-docs/stable/remote_data.html
| @@ -166,7 +165,7 @@ def test_max_median_exposure(self, positions, expected): | |||
| ]) | |||
| def test_detect_intraday(self, positions, transactions, expected): | |||
| detected = detect_intraday(positions, transactions, threshold=0.25) | |||
| assert_equal(detected, expected) | |||
| assert detected == expected | |||
There was a problem hiding this comment.
Why do we use assert here and not one of the pandas assertion functions?
There was a problem hiding this comment.
Seems like assert_equal was removed.
| @@ -53,11 +53,11 @@ | |||
| 'scipy>=0.14.0', | |||
| 'seaborn>=0.7.1', | |||
| 'pandas-datareader>=0.2', | |||
| 'empyrical>=0.2.2' | |||
| 'empyrical>=0.3.0' | |||
| ] | |||
|
|
|||
There was a problem hiding this comment.
Do we also want to pin versions instead of using >=? The current pandas issues were due to this, but there are advantages/problems with both >= and ==.
There was a problem hiding this comment.
Maybe you're right.
There was a problem hiding this comment.
My instinct is to allow a range. Otherwise every environment/application that uses pyfolio must use exactly this version of empyrical. This means we couldn't install zipline with a newer version of empyrical on our research platform, if we wanted newer empyrical features. If we can trust the maintainers of empyrical to keep its API backwards compatible, then we should be ok 😉 .
I'm not sure what the pandas issues were, though, so I might be off base here.
There was a problem hiding this comment.
That makes sense, thanks @richafrank
pyfolio/tests/test_tears.py
Outdated
| @@ -26,7 +26,7 @@ class PositionsTestCase(TestCase): | |||
| test_returns = read_csv( | |||
| gzip.open( | |||
| __location__ + '/test_data/test_returns.csv.gz'), | |||
| index_col=0, parse_dates=True) | |||
| index_col=0, parse_dates=True).iloc[4:] | |||
There was a problem hiding this comment.
Why skip the first 4 rows for only the returns CSV, but not the positions or transactions CSVs?
There was a problem hiding this comment.
those were all 0's. it was part of me tracking down a problem but I think the problem was somewhere else so we can try removing this again.
|
Looks great, thank you @twiecki! |
|
Should probably cut a new release. @richafrank anyone who can help with that? |
There were some problems after updating pandas and pymc3 3.1.