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

Basic support for arbitrary exchange source in key.ratios #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mwaldstein
Copy link

Should address #26

Enables basic fetching of key.ratios / morningstar financial data by splitting the collection of morningstar data from the calculation of valuation ratios.

* morningstar.ratios (tq_get_util_6)
* calcultaion of Valuation ratios
@mdancho84
Copy link
Collaborator

@mwaldstein Code is spot on for providing two options. The big consideration we need to think about is how to ensure the P/E ratio is calculated correctly each time. If this is not possible, then we should not provide this data, and then only one option should be available. If we can do it, I think two options could be "key.ratios" and "key.ratios.pe" or something like this to keep the users on the same page with minor difference between options. What are your thoughts?

@mwaldstein
Copy link
Author

I took the approach of preserving backwards compatibility - for US stocks key.rations functions the same as it always has, while there is the check for currency to ensure that "bad" PE ratios aren't calculated. That happens here - 11fbf93#diff-0f382c8fc5e916469c5d7f034ad656d7R468

As a result, calling tq_get("AAPL", get="key.ratios") returns 7 rows, while tq_get("PINX:UBSFF", get="key.ratios") only returns 6. I feel it would be cleaner to instead return the PE rows, but make them NA.

The overall idea I was going for is that a user would only ever use 'key.ratios' - the second call is just utility in the background, but that PE would be filled if available.

I should add I keep trying to think through a way to break the PE ratio calculation out as a utility function - given that if I need both stock prices and PE, the stock price call gets made twice currently. e.g. something like "calc_pe(stock_prices, financials)".

I kept wanting to find an elegant way to wrap the PE calculation in a utility function

@mdancho84
Copy link
Collaborator

I like that idea if you can get it to work where there's only one call and internally it decides if sufficient information is available and if the output is correct. Two calls for the same functionality seems excessive.

@mwaldstein
Copy link
Author

OK - it is effectively working that way right now - I'll make some basic changes to "hide" the raw call, but my sense of cleanliness suggests keeping the separate logical function (not user facing).

Seems like this is getting close enough that I'll update docs and tests in the next iteration.

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.

2 participants