no symbol prefix on column names

joshuaulrich / rfimport

getSymbols() reboot

16 stars 2 forks source link

no symbol prefix on column names #4

Closed ethanbsmith closed 2 years ago

ethanbsmith commented 3 years ago

a simple xts containing a OHLCVA returned from import_ohlc/getSymbols shodu not have the Symbol as part of the column names

this allows for simple column reference by name without the use of Op(), Cl(), etc
simple rbind after converting to a data.frame/data.table
Symbol prefixing should be part of the merge functionality. see #5
this would work well with name propagation in TTR functions . eg: SMA(SPY$Close, 10) could return CloseSMA10 as a column name

joshuaulrich commented 3 years ago

@ethanbsmith Let me know what you think of the branch I just pushed... I'll merge if it looks good to you.

ethanbsmith commented 3 years ago

@joshuaulrich is the intent for this library to sit on top of existing quantmod as is or to replace it? I would have though t it easier to just remove the prefixing code from getSymbols.xxx. or is this just to prove out the ideas before working on actual code?

joshuaulrich commented 3 years ago

We will not be making any breaking changes to quantmod. This package is a place where we can make all the changes we would like to make, but in a way that won't impact any existing code.

I intend to eventually remove the dependency on quantmod, but continue to use quantmod for the foreseeable future. Then we can focus on the new ideas instead of simply re-writing working code.

ethanbsmith commented 3 years ago

don't think the existing regex will work for symbols with a "." eg: BRK.A, ZEV.W, etc dont we have the actual symbol at this point in the code as that gets passed to getSymbols. maybe something like gsub(paste0("^", Symbol, "\\."), "", colnames()) would be more robust?

ethanbsmith commented 3 years ago

another option would be to add a new symbol.prefix=TRUE parameter to the existing getSymbols implementations that controls the prefixing functionality

joshuaulrich commented 2 years ago

I just pushed a commit that accounts for the issue you brought up. I added a test case for the case you pointed out. Thanks for catching it!

Let me know if this looks good to you and I'll merge.

joshuaulrich commented 2 years ago

I decided to stick with the current approach, even though it uses mind-bending regex. It should work for nearly anything. I'm open to trying to simplifying it once we've used this more and built more test cases.