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.

Sign up for GitHub

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

Jump to bottom

BUG: cal attribute of aggregate rules was not being propagated #2062

Merged
merged 1 commit into from
Jan 27, 2018
Merged

BUG: cal attribute of aggregate rules was not being propagated #2062

merged 1 commit into from
Jan 27, 2018

Conversation

richafrank
Copy link
Member

Otherwise algos crash with

AttributeError: 'BeforeClose' object has no attribute 'cal'

if a function is scheduled with, e.g.

schedule_function(
    func=my_func,
    date_rule=date_rules.every_day(),
    time_rule=time_rules.market_close(minutes=30) & time_rules.every_minute(),
)

@coveralls
Copy link

coveralls commented Dec 26, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 87.58% when pulling ba99851 on event-cal-composition into e4f555f on master.

@ssanderson
Copy link
Contributor

Just to make sure I understand, time_rules.market_close(minutes=30) & time_rules.every_minute(), should be functionally equivalent to just time_rules.market_close(minutes=30), right?

@richafrank
Copy link
Member Author

Yup, semantically there's no reason there for the second term; this was just to show that each of those terms doesn't get assigned a cal attribute.

ssanderson
ssanderson approved these changes Jan 3, 2018
View reviewed changes
Copy link
Contributor

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few small comments, but generally looks good. My general preference would be to not mutate these objects at all after construction, but doing that without breaking the user-facing API would mean we'd have to do something like make the user-facing objects into factories that construct "real" rules given a calendar, which feels enough more complicated that I'm not even sure I'd prefer that here if it were zero extra work to implement.

zipline/algorithm.py Outdated
@@ -1063,7 +1063,7 @@ def fetch_csv(self,

return csv_data_source

def add_event(self, rule=None, callback=None):
def add_event(self, rule, callback=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason callback is allowed to be None here? I would think failing to pass a callback is almost always a bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing it was convenient for tests. Maybe @llllllllll knows more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think there are good defaults for either. We should make them required.

zipline/utils/events.py
@@ -546,11 +557,14 @@ class StatefulRule(EventRule):
def __init__(self, rule=None):
self.rule = rule or Always()

def new_should_trigger(self, callable_):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any call sites.

zipline/utils/events.py Outdated
@@ -236,6 +236,8 @@ def handle_data(self, context, data, dt):


class EventRule(six.with_metaclass(ABCMeta)):
cal = None # A calendar instance is set when scheduling a function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth clarifying that the calendar is set on instances of the rule, as opposed to the class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make cal an abstract property to ensure that all the subclasses implement setters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that as well. My only concern was about the performance of the properties inside the should_trigger implementation, but I didn't validate that, and @lazyval could also solve that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for simple attribute access property will actually be faster than lazyval. lazyval has more complicated machinery in exchange for memoizing the result, but if the fetching the attribute is trivial, then I think property will probably have less overhead.

@richafrank
Copy link
Member Author

Agreed. I went through the same thought process, and settled on this PR.

@coveralls
Copy link

coveralls commented Jan 3, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.04%) to 87.6% when pulling 94fd240 on event-cal-composition into e4f555f on master.

@coveralls
Copy link

coveralls commented Jan 4, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 87.604% when pulling a44a2a4 on event-cal-composition into e4f555f on master.

@richafrank richafrank force-pushed the event-cal-composition branch from a44a2a4 to 704e0cd Compare January 4, 2018 17:00
@richafrank richafrank changed the title BUG: cal attribute of aggregate rules was not being propogated BUG: cal attribute of aggregate rules was not being propagated Jan 4, 2018
@coveralls
Copy link

coveralls commented Jan 4, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 87.616% when pulling 93fa848 on event-cal-composition into fb735e7 on master.

@richafrank richafrank force-pushed the event-cal-composition branch from 704e0cd to 93fa848 Compare January 26, 2018 22:34
@richafrank richafrank merged commit 5c8b588 into master Jan 27, 2018
@richafrank richafrank deleted the event-cal-composition branch January 27, 2018 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssanderson ssanderson ssanderson approved these changes

@llllllllll llllllllll Awaiting requested review from llllllllll

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@richafrank @coveralls @ssanderson @llllllllll