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

clone.orderbook does not strip history #75

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

Conversation

nmatare
Copy link

@nmatare nmatare commented Dec 22, 2017

The clone.orderbook function in paramsets.R does not actually strip history:

The old orderbook is assigned a new name 'cloned.portfolio.st', but the for loop continues to reference the old name, effectively voiding the strip.history = TRUE. Also, by keeping the first order with [1, ] one could potential pass an undesired open order to an attempted set of parameters, "<- list(NULL) solves this issue".

library(quantstrat)

quantstrat:::clone.orderbook
function (portfolio.st, cloned.portfolio.st, strip.history = TRUE) 
{
    orderbook <- getOrderBook(portfolio.st)
    i <- 1
    names(orderbook)[i] <- cloned.portfolio.st # !! # we change name here
    if (strip.history == TRUE) { # !! # but still attempt to reference old names in the loop
        for (symbol in names(orderbook[[portfolio.st]])) orderbook[[portfolio.st]][[symbol]] <- orderbook[[portfolio.st]][[symbol]][1, ] # keep potentially open orders
    }
    put.orderbook(cloned.portfolio.st, orderbook)
}

Example:


library(quantstrat)
options(width = 250)
 
# INIT FABER
FinancialInstrument::currency('USD')
FinancialInstrument::stock(primary_id = "IBM", currency = "USD", multiplier = 1)    
# Sys.setenv(TZ = "UTC")
 
initPortf('faber', symbols="IBM")
initAcct('faber', portfolios='faber', initEq=100)
initOrders(portfolio='faber')
strategy("faber", store=TRUE)
add.indicator('faber', name = "rollapply", 
    arguments = list(data = quote(Cl(mktdata)), width=10, FUN=mean, fill = 0), label="SMA10")
add.signal('faber',name="sigCrossover",
    arguments = list(columns=c("Close","SMA10"),relationship="gte"),label="Cl.gt.SMA")
add.signal('faber',name="sigCrossover",
    arguments = list(columns=c("Close","SMA10"),relationship="lt"),label="Cl.lt.SMA")
add.rule('faber', name='ruleSignal', 
    arguments = list(sigcol="Cl.gt.SMA", sigval=TRUE, delay = 10, orderqty=500, 
             ordertype='market', orderside='long', pricemethod='market',TxnFees=-5), 
     type='enter', path.dep=TRUE)
 
add.rule('faber', name='ruleSignal', 
    arguments = list(sigcol="Cl.lt.SMA", sigval=TRUE, orderqty='all', ordertype='market', 
             orderside='long', pricemethod='market',TxnFees=-5),
     type='exit', path.dep=TRUE)
## END FABER
 
getSymbols(
    Symbols="IBM", 
    src="yahoo", 
    from=as.Date("2013-01-01"), 
    to=as.Date("2013-04-05"), 
    auto.assign = TRUE
)
 
applyStrategy(
    strategy='faber', 
    portfolios='faber'#,
    # rule.subset = "2014-12-23/2015-03-07"
)

quantstrat:::clone.orderbook('faber', 'faber.test', strip.history = TRUE)
all.equal(getOrderBook('faber')$faber$IBM, getOrderBook('faber.test')$faber.test$IBM)
[1] TRUE

…AND it kept first order in orderbook; if order was open this could produce undesirable results
…was method used; thus any changes to the clone resulted in changes to the original
@braverock
Copy link
Owner

braverock commented Dec 23, 2017

Your second commit makes perfect sense to me to use list2env.

I don't think your first commit makes sense. Just because we are splitting time can't mean that any open orders won't be processed. I believe that we need open orders to carry forward. If strip.history is TRUE, then maybe it makes sense, but it isn't clear from the patch that it only applies in that case?

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