plot.xts ignores col.up and col.dn for type="h"

[charliebone]

Description

When plotting a histogram, plot.xts does not respect the up.col and dn.col arguments, instead it always seems to take the color from the col argument.

Minimal, reproducible example

library(xts)
data(sample_matrix)
x <- as.xts(sample_matrix, dateFormat="Date") - 48

# using default arguments explicitly for clarity
plot(x[,4], type = "h", up.col = "green", dn.col = "red") 

# providing explicit colors to show desired output
cols <- ifelse(x[,4] > 0, "green", "red")
plot(x[,4], type = "h", col=cols)

[joshuaulrich]

I think the central problem is that we set default values for non-user-specified arguments too early. We currently set default values before we create the expression that will be evaluated to create the plot. Instead, we should wait to set default values until the expression that creates the plot is evaluated.

This is a good general rule for all plot.xts() functionality, not only this particular issue.

In this specific case, we need to delay setting values for col until we're inside chart.lines(). I realize that may be more difficult than a quick hack, so I'm open to something hack-ish until we can get a more robust and general solution worked out.

[rossb34]

I agree that we could do a better job of setting default values, but deciding when imposes an implicit time dependency which could make things more complicated.

My preference is for an approach with a simple set of rules. The up.col and dn.col variables are only used in chart.lines for type="h", but are set by default and passed around in every chart.lines call. col, up.col, and dn.col are specified as arguments with default values so they will always be defined, unless of course the user explicitly passes in a value for those arguments. I propose that we change the default values for up.col and dn.col to NULL. Then we only use those to chart the histogram if up.col and dn.col are not NULL, i.e. specified by the user.

[joshuaulrich]

Can you clarify what you mean by "implicit time dependency"?

I think your preferred solution would be a fine patch to get this working again. Go ahead and give it a shot.

[rossb34]

Can you clarify what you mean by "implicit time dependency"?

Arguments passed in from the function are just that, arguments, whether they have the default values or user specified values. They live in the cs$Env environment. Deciding to set them before or after something happens is an implicit dependency. Perhaps you had something else in mind regarding setting non-user-specified arguments too early. I interpreted your comment as something along the lines of setting default values after x occurs.

[joshuaulrich]

My point was that we "set" many values when we build the call to chart.lines(). For example (relevant portion below):

exp <- quote(chart.lines(xdata, type=type, lty=lty, lwd=lwd, lend=lend,
                         col=theme$col, up.col=theme$up.col, dn.col=theme$dn.col,
                         legend.loc=legend.loc))

So the values for type, lty, lwd, lend, etc are all "set" at this point. So when chart.lines() is actually evaluated, there's no way to tell if those values were set as defaults or set by the user. That means you need to check for user-specified values and set any defaults before you construct the chart.lines() call.
At minimum, it would be less redundant to set defaults inside chart.lines() rather than outside, when creating the chart.lines() call (e.g. use "theme" values unless an argument specifically overrides them).

[rossb34]

This issue requires 2 fixes:

1. respect the up.col and dn.col arguments when not null
2. the col argument is recycled

For the second point, we need to handle 3 cases (assuming up.col and dn.col are both NULL).

1. col=NULL
   a. Set the color to 1 so we plot something
2. length(col) != nrow(x)
   a. This is the most likely case because of the default col=1:8. For example, calling plot(edhec[,1], type="h") is currently handled as plotting {element, color} => [{1,1}, {2,2}, ..., {8,8}] and then the col vector is recycled such that [{9, 1}, {10, 2}, ...]. I don't think this is the behaviour we want. If the length of the color vector is less than the number of observations of x, then we should just use the first element of the color vector.
3. length(col) >= nrow(x)
   a. Suppose the user specifies col for each element. For example, the first 20 elements black, the next 20 elements red, and the rest of the elements as gray. In this case, we should just use the color vector passed in.

[joshuaulrich]

Merged to master, closing as fixed.