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

More benchmark fixes #2762

Merged
merged 3 commits into from
Aug 24, 2020
Merged

More benchmark fixes #2762

merged 3 commits into from
Aug 24, 2020

Conversation

richafrank
Copy link
Member

Fixes #2761 and adds the docstring missing in #2627 (comment).

@richafrank richafrank requested a review from llllllllll August 23, 2020 19:44
@coveralls
Copy link

coveralls commented Aug 23, 2020

Coverage Status

Coverage increased (+0.006%) to 88.255% when pulling 94841eb on more-benchmark-fixes into e8cd422 on master.

if benchmark_returns is not None:
expected_perf = self.expected_perf[example_name]
else:
# With no benchmark, expect zero results for these metrics:
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 precompute this in the init like self.no_benchmark_expected_perf?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Done.

@@ -513,7 +515,8 @@ def resolve(self, asset_finder, start_date, end_date):
benchmark_returns = None
except SymbolNotFound:
raise _RunAlgoError(
"Symbol %s as a benchmark not found in this bundle."
"Symbol %r as a benchmark not found in this bundle."
% self.benchmark_symbol
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@llllllllll
Copy link
Contributor

lgtm

@richafrank richafrank force-pushed the more-benchmark-fixes branch from 90d4fb7 to 94841eb Compare August 24, 2020 14:05
@RichardDale
Copy link

Given this is such a common-use case I would suggest this probably warrants a v1.41 release? It's certainly been a common topic from our user base.

@richafrank
Copy link
Member Author

Given this is such a common-use case I would suggest this probably warrants a v1.41 release? It's certainly been a common topic from our user base.

👍 I was thinking the same thing.

@richafrank richafrank merged commit 47b5f50 into master Aug 24, 2020
@richafrank richafrank deleted the more-benchmark-fixes branch August 24, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue (and solution) for running run_algorithm without benchmark_returns
4 participants