tidyquant

tidyquant copied to clipboard

Should address https://github.com/business-science/tidyquant/issues/26

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

@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?

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 - https://github.com/business-science/tidyquant/pull/45/commits/11fbf93e080074671bed3bca5e9a8f902ee3d31c#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

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.

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.