braverock / PerformanceAnalytics

207 stars 105 forks source link

table.DownsideRiskRatio/table.Variability variable freq not defined #128

Open ThomasHehehehe opened 5 years ago

ThomasHehehehe commented 5 years ago

Hello, in table.DownsideRiskRatio function: freq is not defined if scale is not NA.

y = checkData(R) columns = ncol(y) columnnames = colnames(y) if (is.na(scale)) { freq = periodicity(R) ... } for (column in 1:columns) { ... znames = c(paste0(freq$scale, " downside risk"), "Annualised downside risk", "Downside potential", "Omega", "Sortino ratio", "Upside potential", "Upside potential ratio", "Omega-sharpe ratio") ... }

braverock commented 5 years ago

thanks for the report.

Could you describe why you think this is a problem? If you don't pass scale, the function tries to figure it out.

Do you have an example of a problem you're having?

I'm happy to take a look, but I want to know what you were expecting, and what, if anything, is not working.

ThomasHehehehe commented 5 years ago

Thank you for quick response!

table.DownsideRiskRatio(port, scale = 252, digits=4) does not work while table.DownsideRiskRatio(port, digits=4) works.

I checked the function and found that if scale is defined, and the code dose not go into if{} , therefore freq will not be defined. However, freq is used anyway in for{} in following line: znames = c(paste0(freq$scale, " downside risk"), "Annualised downside risk",

Thank you! PerformanceAnalytics is a very nice package.

ThomasHehehehe commented 5 years ago

version: PerformanceAnalytics_1.5.3

braverock commented 5 years ago

upon investigation, your report doesn't make sense to me, and I still dont have a reproducible example.

The code in question is this:

    if(is.na(scale)) {
        freq = periodicity(y)
        switch(freq$scale,
            minute = {stop("Data periodicity too high")},
            hourly = {stop("Data periodicity too high")},
            daily = {scale = 252},
            weekly = {scale = 52},
            monthly = {scale = 12},
            quarterly = {scale = 4},
            yearly = {scale = 1}
        )
    }

so freq is defined if scale is not defined, and it is only used inside that if block, not any where else in the function.

In all cases, scale will be defined, and scale is what is used later in the function.

I suspect that your port object is non-conforming in some way, but without a reproducible example demonstrating the problem you say you're having, I can't demonstrate that.

ThomasHehehehe commented 5 years ago

Hi! Thanks for following up! Actually, in the script for table.DownsideRiskRatio(), freq is used outside the if block. https://github.com/braverock/PerformanceAnalytics/blob/faf539c7013995ac9c947d7c1abedc681efd18aa/R/table.DownsideRiskRatio.R#L61-L72

In line 64, freq$scale is used. This causes the bug if scale is well defined in the parameter input. Hope I make myself clear. Thank you!

braverock commented 5 years ago

OK, now I understand you. I will fix.

ThomasHehehehe commented 5 years ago

OK, now I understand you. I will fix.

Thank you! Similar issues may also happen in other functions using scale input.

DzimitryM commented 4 years ago

Please fix the same bug in function 'table.Distributions()'

It can be done by just placing the following string outside the block 'if(is.na(scale)) {...}' freq = periodicity(y)