search

jonathancornelissen / highfrequency

The highfrequency package contains an extensive toolkit for the use of highfrequency financial data in R. It contains functionality to manage, clean and match highfrequency trades and quotes data. Furthermore, it enables users to: calculate easily various liquidity measures, estimate and forecast volatility, and investigate microstructure noise and intraday periodicity.
147 stars 63 forks source link

Import `FKF::fkf` or use it conditionally in `stochPer()` and `loglikBM()` #88

Closed joshuaulrich closed 2 years ago

joshuaulrich commented 2 years ago

The FKF package is in 'Suggests', but used unconditionally in two places in internalSpotVolAndDrift.R (line 243 and line 272). That causes those function calls to fail if {FKF} is not installed (as was the case when I was running reverse dependency checks for quantmod).

It's good practice to either import that function, or only use it if it's installed. Here's how you could do either one:

  1. Add importFrom(FKF, fkf) to the NAMESPACE file, or
  2. Add if (!requireNamespace("FKF", quietly = TRUE)) stop("please install the FKF package to use this function") to the beginning of each function that uses FKF::fkf().
onnokleen commented 2 years ago

Hi Joshua, Thanks for the feedback. It is addressed in the most recent release from yesterday. Best, Onno @kboudt: I think you can close this issue then

joshuaulrich commented 2 years ago

Thanks @onnokleen! I looked at your commit and it looks like you added the check on line 243, but I don't see one on line 272 (the loglikBM() function). FWIW, I don't think it's worth another CRAN release if you just pushed an update.

onnokleen commented 2 years ago

That loglikBM() should only be called after checking for FKF on line 188 so there isn't any need to check again. But after I searched in other files, I found the dependency popping up somewhere else -.- I'll add that to my todo list :)

onnokleen commented 2 years ago

That loglikBM() should only be called after checking for FKF on line 188 so there isn't any need to check again. But after I searched in other files, I found the dependency popping up somewhere else -.- I'll add that to my todo list :)

Forget the last part, I think that was only in an old version. I'll double check later just to be sure.