braverock / PerformanceAnalytics

209 stars 105 forks source link

Refactored charting functions throw warnings when called with default args #35

Open evelynmitchell opened 8 years ago

evelynmitchell commented 8 years ago

Many of the recently refactored charting functions throw numerous warnings when called with their default arguments. Here are all the function calls I tested:

chart.Drawdown(managers[,1])
chart.CumReturns(managers[,1])
chart.Bar(managers[,1])
chart.BarVaR(managers[,1])
charts.PerformanceSummary(managers[,1])
chart.RelativePerformance(managers[,1], managers[,9])
chart.RollingPerformance(managers)
chart.RollingMean(managers)
chart.RollingCorrelation(managers[,1], managers[,2])
chart.RollingRegression(managers[,1], managers[,2])
charts.RollingRegression(managers[,1], managers[,2])

All the calls threw warnings similar to this:

chart.Drawdown(managers[,1]) 
Warning messages:
1: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The auto.grid argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
2: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The las argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
3: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The ylab argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
4: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The xlab argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
5: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The cex.axis argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
6: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The cex.lab argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
7: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The cex.labels argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
8: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The cex.main argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.
9: In chart.TimeSeries.base(R, auto.grid, xaxis, yaxis, yaxis.right,  :
  The xaxis.labels argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.

There should only be a warning if a user explicitly supplies one of the deprecated arguments. And possibly consider removing the deprecated arguments from the function formals (the docs should note that they're deprecated as well).

joshuaulrich commented 8 years ago

You created 11 issues about these warnings, but you do not say what the problem is in any of the issues. Can you please elaborate?


EDIT: I have updated the initial report with details from private conversation.

braverock commented 8 years ago

The key issue here seems to be the second clause of the if test for the warning. they are all of the type:

# deprecated arguments if(hasArg(auto.grid) || !isTRUE(auto.grid)) { warning("The auto.grid argument of chart.TimeSeries has been deprecated, and may be removed in a future release, see help('chart.TimeSeries') for more information.") }

the second half, after the or || clause, will always trigger, because the argument has a default setting in the function signature.

I think that only using hasArg will resolve this, because the warning will only be triggered if user code sets one of these options.

We should be able to remove code that uses these options from inside the package, e.g. in the other chart.* functions.

joshuaulrich commented 8 years ago

We should also not throw a warning if the user supplies a par name as an argument. I.e., one of:

R> graphics:::.Pars
 [1] "xlog"      "ylog"      "adj"       "ann"       "ask"       "bg"        "bty"      
 [8] "cex"       "cex.axis"  "cex.lab"   "cex.main"  "cex.sub"   "cin"       "col"      
[15] "col.axis"  "col.lab"   "col.main"  "col.sub"   "cra"       "crt"       "csi"      
[22] "cxy"       "din"       "err"       "family"    "fg"        "fig"       "fin"      
[29] "font"      "font.axis" "font.lab"  "font.main" "font.sub"  "lab"       "las"      
[36] "lend"      "lheight"   "ljoin"     "lmitre"    "lty"       "lwd"       "mai"      
[43] "mar"       "mex"       "mfcol"     "mfg"       "mfrow"     "mgp"       "mkh"      
[50] "new"       "oma"       "omd"       "omi"       "page"      "pch"       "pin"      
[57] "plt"       "ps"        "pty"       "smo"       "srt"       "tck"       "tcl"      
[64] "usr"       "xaxp"      "xaxs"      "xaxt"      "xpd"       "yaxp"      "yaxs"     
[71] "yaxt"      "ylbias"

They should just be passed via ... to plot.xts.

tchevri commented 7 years ago

Sorry to bother - I do so, because I think this package is absolutely terrific and i am very eager to keep using it - it's everything i like and hope for!... However, this "warning" deluge thing is throwing my coding off - as I am trying to use knitr .Rnw which makes it a little more finicky than just using the package in R, if I am allowed to say so... Makes it a lot harder to ignore all the warnings...

"the second half, after the or || clause, will always trigger, because the argument has a default setting in the function signature"... indeed, that is what i found out too when i ran the code line by line... as i was wondering if i was just stupid and not using the function properly... and as someone pointed out, we're referred to looking at the help F1 regarding the warnings - but the help has absolutely zero information indeed - as a matter of fact, i run the sample code provided to use the function and that throws the same warnings. a desirable outcome? shouldn't sample code work smoothly!?

So i would like to ask - these warnings - do they add any purpose here? are they necessary? I thought this was something temporary - and it would be fixed, but it's been now more than half a year., which gives me the courage to chime in I feel bad complaining about this as i am just a free-user of your great work, but the truth is that i like your work and do want to be able to enjoy it! Therefore, may i dare asking you to kindly fix this - so that these (absurd?) warnings do not show or that at least there is a work around and able to use the function without the warnings?

fyi, there was another v interesting piece of work (see "plot.xts is wonderful" article that comes as a v high hit in google search, but is now just not working at all), called xtsExtra that seems to also have been killed with that same kind of change - adding "deprecated" everywhere.... i hope that for the Sunday user like me, this can be avoided as it's very unsettling when we run sample code and it fails and we just don't have the resources to fix it, like you the top pros. Many thanks, thomas

braverock commented 7 years ago

The warnings are just warnings. They are sloppy and incomplete code on the part of our GSoC student @naturalsmen , so I'd like to fix them. That said, I've got other priorities, and this is one of a very long list of things to do.

Patches welcome that check for actual use of deprecated or removed parameters, while not triggering on function signature defaults. I suspect it will be a simple check using hasArg(), but as I said I haven't spent time on this yet.