braverock / quantstrat

289 stars 114 forks source link

apply.paramset - error handle edge cases where zero trades are returned #121

Closed jaymon0703 closed 4 years ago

jaymon0703 commented 4 years ago

See https://stackoverflow.com/questions/61987166/no-results-from-apply-paramset-if-one-parameter-combination-returns-nothing for an example. If a param.combo results in no signals being fired and/or no rules executing trades, then r$tradeStats is empty and an error thrown in 586-589.

if(is.null(results$tradeStats)){
results$tradeStats <- cbind(r$param.combo, r$tradeStats)
} else {
results$tradeStats <- rbind(results$tradeStats, cbind(r$param.combo, r$tradeStats))
}
jaymon0703 commented 4 years ago

Using the code from the SO link above as reference, where there are 2 param combos, we are calling tradeStats() from the blotter package once with a portfolio with trades, and once without. The result from calling tradeStats() on a portfolio with trades gives an output with these colnames:

image

The resulting colnames from calling tradeStats() on a portfolio with no trades is different in one colname (Total.Net.Profit) and the order of some colnames, see below:

image

So when we call rbind() with those 2 objects there is a name mismatch resulting in the below error.

match.names(clabs,names(xi)) names do not match previous names

So we either need a solution in blotter::tradeStats() or in quantstrat::apply.paramset(). I am tending toward a solution having to be implemented in blotter::tradeStats().

scottwcclark commented 4 years ago

Hi Jasen

I found the same thing too!

I looked into tradeStats and it looks like the reason the data with transactions returns "Net.Trading.PL" is because the TotalNetProfit object is created using the following (line 236): TotalNetProfit <- last(Equity) Which returns an xts object with the column name "Net.Trading.PL".

Would adding this after the line above be ok do you think? names(TotalNetProfit) <- "Total.Net.Profit"

Also there is a slight difference in the order as you mentioned above. In the ret dataframe, "Std.Err.Trade.PL" is after "Med.Trade.Losses" whereas further down in the code for the tmpret dataframe it is after the "Std.Dev.Trade.PL".

Perhaps matching the same order as the tmpret would be a good solution? image

braverock commented 4 years ago

Net.Trading.PL and Total.Net.Profit are slightly different. Net.Trading.PL is, as stated, from trades, and Total.Net.Profit is the change in equity value of the Account.

Net.Trading.PL is expected a number of other places in the code, so we probably want to make sure that Net.Trading.PL is available in the returned results.

jaymon0703 commented 4 years ago

Brian is right. There are lots of places relying on Net.Trading.PL.

image

[EDIT:] Adding screenshot for lines of code referencing Net.Trading.PL in blotter

image

The problem does not originate in tradeStats but rather in apply.paramset since we are trying to call tradeStats on a portfolio with no trades which kind of does not make sense. Of course a person would consider such a scenario for param.combos an edge case since you want to be optimizing scenarios that actually result in trades. I now think we should handle the error in apply.paramset and i will be submitting a change for that function.

@scottwcclark feel free to submit a PR to blotter for re-arranging the order of ret?

jaymon0703 commented 4 years ago

The challenge of course being that we run the strategies with different param.combos using foreach which we cannot simply exit from (eg. using next) when we encounter this scenario.

scottwcclark commented 4 years ago

@jaymon0703 I've created a new pull request: https://github.com/braverock/blotter/pull/105 (Let me know if this looks right as this is the first pull request I've submitted)

jaymon0703 commented 4 years ago

@jaymon0703 I've created a new pull request: https://github.com/braverock/blotter/pull/105 (Let me know if this looks right as this is the first pull request I've submitted)

Thanks @scottwcclark for the PR and @braverock for the merge. I will keep this issue open until we solve for the error handling for param.combos that results in no trades.

finarb commented 4 years ago

changed the code in

results$tradeStats <- rbind(results$tradeStats, cbind(r$param.combo, r$tradeStats))

to this

cols = colnames(cbind(r$param.combo, r$tradeStats))
cols = gsub('Net.Trading.PL','Total.Net.Profit',cols)
tempdf = cbind(r$param.combo, r$tradeStats)
colnames(tempdf) =cols
results$tradeStats <- rbind(results$tradeStats, tempdf)
jaymon0703 commented 4 years ago

Thanks @finarb that appears to be a neat workaround, although i have not tested it. We are thinking of handling scenarios in which a param.combo returns no trades a bit more gracefully. This will likely be my next OSS focus, together with adding functionality to apply.paramset to parallelize applyStrategy.

jaymon0703 commented 4 years ago

Will merge this feature branch to master based on the feedback in https://github.com/braverock/blotter/pull/106#issuecomment-679332989.