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

Added variable resolution and risk free rate to the back-test metrics #63

Merged
merged 4 commits into from
Oct 30, 2021

Conversation

jkalish14
Copy link
Contributor

Adds variable trading-period capability to the calculation of the metrics. The value is driven off the "resample_account_value_for_metrics" property in the backtest.json file.

Also added "risk_free_return_rate" to the file so the user can specify their value. Added the value into the defaults to ensure error checking.

left a TODO in regards to changing the values when strategies are setup to close all positions over night; not quite ready to tackle this myself yet.

@EmersonDove
Copy link
Member

This is great - I'm going to spend a bit of time before merging this to add the new settings to docs and get a review!

Copy link
Contributor

@bfan1256 bfan1256 left a comment

Choose a reason for hiding this comment

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

Overall looks great! This allows the metrics to handle the case where values are not being resampled to 1d and instead another specific resolution. Phenomenal work with this fix! Thank you so much!

@@ -836,6 +836,9 @@ def search(arr, size, x):
# Find the interval second value
interval_value = time_interval_to_seconds(resample_setting)

# Add the internval value to dictionary
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight typo here

number of trading periods in each year

"""
# TODO: update logic to account for strategies that do not hold overnight
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, I think this is perfectly fine. This is a really valuable function especially if they put in a different resolution

Copy link
Contributor

@bfan1256 bfan1256 Oct 29, 2021

Choose a reason for hiding this comment

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

Detecting no holds after market close is a little difficult on our end and we could theoretically update this in the case of the interval that they want to do (either based on an additional settings preference). Also, we should probably also detect if it's on a crypto or 24/7 exchange so it's 365 instead of 252. @EmersonDove , thoughts on how we should do this? Perhaps passing in an additional parameter of days?

@@ -71,7 +71,8 @@
"continuous_caching": True,
"resample_account_value_for_metrics": "1d",
"quote_account_value_in": "USD",
"ignore_user_exceptions": False
"ignore_user_exceptions": False,
"risk_free_return_rate" : 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!!

@jkalish14
Copy link
Contributor Author

saw that the build failed; is there something i need to fix here?

@EmersonDove
Copy link
Member

EmersonDove commented Oct 29, 2021

I'm not sure I'm looking into it now. It seems like the .json files changed on our end but I don't believe they've been updated. Your branch passes fine locally so I think it's some update we made.

@EmersonDove
Copy link
Member

EmersonDove commented Oct 30, 2021

It looks like GitHub just doesn't pass the test keys in from fork PRs. I think your code is good to go I'll just update docs and give it a merge.

@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@EmersonDove EmersonDove changed the base branch from main to development October 30, 2021 00:11
@EmersonDove EmersonDove merged commit fd03875 into blankly-finance:development Oct 30, 2021
@EmersonDove EmersonDove mentioned this pull request Oct 30, 2021
@jkalish14 jkalish14 deleted the jck_dev branch December 11, 2021 16:10
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.

3 participants