-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
BUG: Fixed daily price ffill using today's close #2074
Conversation
Will look into the py3 failures. -edit- Fixed. |
9ea3ef9
to
db36fa9
Compare
# volume in today's minute bars yet, we need to use the | ||
# previous day's ffilled daily price. Using today's daily price | ||
# could yield a value from later today. | ||
history_start -= pd.Timedelta(days=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this isn't a trading calendar day?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the most recent close as of the weekend is the same as the most recent close as of Friday, I didn't bother to figure out which trading calendar to use here, especially if there are multiple assets. Do you think this breaks under certain conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it breaks here.
Where it would be relevant is if we had a calendar with a session spanning multiple days, but in practice, I do not think we have that concern.
when we haven't seen any minute volume yet today
db36fa9
to
1c8e0ae
Compare
tests/test_history.py
Outdated
self.assertEqual(len(window), bar_count) | ||
|
||
if not day_idx and idx + 1 < 10: | ||
self.assertTrue(np.isnan(window[-1])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't block on it, but I would recommend picking a few specific times and expected values, crafted to test each condition we expect. That would 1) speed up the test time 2) make the narrative of what is being tested and why more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. In most cases I'd recommend the specific Timestamps, i.e. hardcoding pd.Timestamp('2017-01-09 10:44'
); however since this test is iterating over multiple days, I agree with using the index to select the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, in this case, I saw the important information as being the open or close or the temporal relationship to the open, etc, as opposed to exact times, so I represented them that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests/test_history.py
Outdated
self.assertEqual(len(window), bar_count) | ||
|
||
if not day_idx and idx + 1 < 10: | ||
self.assertTrue(np.isnan(window[-1])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded 👍
when we haven't seen any minute volume yet today