-
Notifications
You must be signed in to change notification settings - Fork 203
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
Feature/beta parameter #84
Feature/beta parameter #84
Conversation
Thank you so much for your past contributions and this big one! It's a big help. Don't worry about the quandl error, you can ignore those for the time being. I'll have a closer look at it over the next few days. Once this is merged, I'll create a new release for FinQuant. |
Excellent, thank you! Please take your time and let me know if I should change something. Best |
d1a50f0
to
f08dc69
Compare
Great addition @PietropaoloFrisoni. Mainly just some minor changes, e.g. adding comment or removing empty line. With the introduction of the class One additional change would be great for this PR: Could you add this as an example? You could make some modifications to |
typo in comment describing Market instance in the portfolio Co-authored-by: Frank Milthaler <fmilthaler@users.noreply.github.com>
Adding decorator to set the market index Co-authored-by: Frank Milthaler <fmilthaler@users.noreply.github.com>
removing empty line Co-authored-by: Frank Milthaler <fmilthaler@users.noreply.github.com>
@fmilthaler Hi, I'm following your suggestions and updating with commits. However, the checks are currently failing due to quandl. At line 38 of the build (3.11) test, I see that: quandl.errors.quandl_error.LimitExceededError: (Status 429) (Quandl Error QELx01) You have exceeded the anonymous user limit of 50 calls per day. To make more calls today, please register for a free Nasdaq Data Link account and then include your API key with your requests. I may be missing something, but it looks like this is the source of the error. Any idea how to solve this? |
By the way, I agree it is very reasonable to create an |
@fmilthaler Great, tests work now. I'll work on the |
Thank you, that would be most helpful for the users (including myself) to see how to use it. :) |
Hi @fmilthaler, I updated the example as well. Please tell me if there's something you would like to change. Best |
@PietropaoloFrisoni Amazing work, I really appreciate all the work you put in here!!! You are a star. Just one minor fix I flagged. Once that is in, this can be merged (and another release is going to happen) :) |
That's way too kind. Thank you so much. Maybe I'm missing something, as I cannot see the flagged minor issue. Could you please point that out (again)? |
That is weird. I commented on it again and tagged you. In case it still does not show up for you:
Only change in that (long) line is the addition of |
Done. Thanks for pointing that out! |
Adding computation of the beta parameter for the portfolio. In the Capital Asset Pricing Model (CAPM), the beta parameter defines how risky the portfolio is compared to the market.
Important: I could not run the old tests due to the usual quandl error (although I obtained my API key). I still don't know what's happening, but I'm working on it. The old tests should be run as a sanity check before merging the pull request.
However, the additional feature is written so that all the previous codes and tests should still work: suppose the argument "market_index" is not explicitly passed to the "build_portfolio" function. In that case, the Market class instance is not created, and the beta parameter of the portfolio is not computed or printed in properties.
@fmilthaler if there's something you would like me to change, please tell me.
All the best