• add @noRd to functions that are not exported
• improve description

There is a lot of redundancy across bv_lambda, bv_alpha and bv_dummy.

Psi is not yet added to log(ML) and does not yet have scale and shape parameters in the code. The prior is and inverse gamma with scale and shape defaulting to 0.02^2.

No need to have this (what amounts to) twice.

While running the code in parallel works nicely (and some outputs work well) the setup is a bit clunky. We should be straightforward to provide a wrapper for this.

At the moment only plot() and print() exist for the bvar object.

Important standard methods are:

• fitted(): Probably better to calculate ex-post.
• predict(): This is currently done in the main chain, as it shouldn't necessitate too much additional effort. Allowing to calculate this ex-post should not be an issue, however.
• summary(): This really needs to be readded.
• residuals(): See fitted()

Further potential methods are:

• coef(): Should be useful and rather straightforward. Optionally consider the vars implementation of Acoef() and Bcoef().
• vcov(): Should also be straightforward.
• logLik(): Might as well.
• irf(): See predict().
• fevd(): Nice shorthand.
• stability(): Additional dependency, but might make sense.
• Phi() & Psi(): Would address facilitating switches from and to vars.

• The user interface of BVAR has bvar as the main fitting function
  returning an object that only has a plot() and a print() method. First,
  other standard methods like fitted(), predict(), residuals(), and
  summary() should be added. Second, methods that are typically available
  for frequentist models like coef(), vcov(), logLik() etc. might also make
  sense in this Bayesian setting. Third, the "vars" package defines generics
  and methods for a lot of VAR-specific functionality, e.g., Phi(), Psi(),
  fevd(), irf(), stability(). As already mentioned in the previous point it
  would be very natural to provide methods to these generics as well.

There seems to be some interest for this and estimation-wise the implementation should be feasible. However, analysis and some potentially important details may diverge to the point where it does not make sense to add this feature. E.g. dummy-observation priors would probably not be compatible.

Add the paper as vignette, maybe adapt a second, more concise demonstration from it (possibly in HTML).

The sign restrictions and psi can't be checked (dimension-wise) within the constructor functions - this needs to be done at the start of bvar.

At the moment the core functionality is tested in the examples, with some (longer) parts only being run when requested specifically.
It makes sense to (a) split examples and tests to a certain extent and (b) extend the unit tests to as much functionality as possible and look more closely at the results.

To achieve this we have to rework the examples and add unit test scripts, possibly using testthat.

Include an interesting dataset to showcase the functionality in a "non-trivial way".

Allow to specify a type in print.bvar.

Possibly allow fcast and irf as types?

Probably the most computationally-intensive part - think through thoroughly.

The prefix bv_ makes it rather easy to find functions associated with the package and allows for some extra options without parameter-clutter. However, it is non-standard and might prove to be an entry-barrier, especially in conjunction with standard S3 methods.

Due to compatibility the functions need to stay anyway. It might make sense to move the standard functionalities to classic names and keep the naming scheme as shortcuts and for specific functions (e.g. prior creation and other settings).

CRAN reports dependences as R (>= 3.5.0). However, the package was tested on 3.3.0 and the DESCRIPTION file lists R (>= 3.3.0).

This appears to be some kind of bug (either of the package or on CRAN) and should be fixed in the future. Afaik the dependency should be taken straight from the DESCRIPTION.

So far the following plots exist:

• Trace
• Density / Histogram
• IRF / Forecast

Trace & Density are wrapped in the (presumably) default print method. However, both should probably be readily accessible on their own (i.e. provide a wrapper).
The IRF and Forecast functions are very similar and in their core they could be generalised and merged.

The naming scheme should also remain consistent (bv_ atm). Plots could be called bv_plot_trace, bv_plot_density, bv_plot_irf, etc.?

Assigning classes to elements of the main object might also be an option for plotting - e.g. one could call plot(object$irf) or plot(object$hyper).

There should be no need for iterating fits over all of the betas. Just use beta-quantiles instead.

The plotting method of our density class now supports plotting the coefficient densities. This should also be possible for the standard plotting method.

It might make sense to reconsider the naming scheme -- we currently have parameters vars, vars_impulse, vars_response, var_dep (dependent), vars_ind (independent).

I guess we should call all of the latter ones the same way (established one is preferable and interpretation of dependent and response variable is similar).
vars is a bit weird, as it is used for the variables to forecast and for hyperparameters to plot...

FRED-QD is now available under a modified ODC-BY 1.0.
Some of the (so-far) excluded series are also in public domain and can be readded. Also notices and the license need to be added.

Several methods (e.g. summary, print, et cetera) need to be implemented for the following classes:

• bvar
• bv_priors
• bv_metropolis
• bv_fcast
• bv_irf

Thanks,

Please always write references in the form
authors (year) doi:...
authors (year) arXiv:...
authors (year, ISBN:...)
with no space after 'doi:', 'arXiv:' and angle brackets for auto-linking.

Please provide small executable examples in all your exported functions'
Rd files to illustrate the use of the exported function but also enable
automatic testing.

Please fix and resubmit.

Best,
Martina Schmirl

Add a print method including visualisation of the sign restrictions.

E.g.:

- / - / +
0 / 0 / -
+ / - / 0

Maybe we should wrap outputs into coda::mcmc objects and just drop the relevant methods from BVAR.

I don't think it's necessary to include them and they just make it look chaotic

I got the feedback that it's a bit counterintuitive to specify the total number of draws and then the amount of them which are to be discarded instead of specifying the number of saved draws and the ones to burn.

Also, when n_draw == n_burn, an error message stemming from the integer check of n_thin pops up at the moment. Whether or not the specification of draws is changed, this should be fixed.

I missed this.

• conditional
• unconditional

Add latest changes to NEWS and cran-comments.

Move formatted input and outputs to TEX.

Basically extend print() with the paths of some (or all) variables?

So I can be even lazier when testing.

• As discussed there's the stationarity question
• If a mode is provided we get an unnecessary warning: priors[["psi"]][["mode"]] == "auto". Should be an easy fix.

This should be easy, as an internal function already exists.

Accessing parameters and variables via vars, vars_impulse and vars_response is still quite weird and non-standard. This really needs to be unified.

Use a variable parameter to limit the variables plotted:

• For impulse responses one might want to look at specific impulses and responses
• Forecasts would jutst be limit to specific variables

Both functions could also use a parameter to limit the plotted horizon.

When doing:

> plot((x$fcast))
Error in plot.window(...) : need finite 'xlim' values
In addition: Warning messages:
1: In min(x) : no non-missing arguments to min; returning Inf
2: In max(x) : no non-missing arguments to max; returning -Inf
3: In min(x) : no non-missing arguments to min; returning Inf
4: In max(x) : no non-missing arguments to max; returning -Inf

this happens with no fcast computed and needs to fail in a nicer way.

In the paper we used the SP500, which is not under public domain and thus no longer included in the package.

Functions and methods have slightly exploded.

We need to make sure that related functions and methods are documented together (e.g. print.bvar method probably fits better to bvar than on its own., irf may as well be grouped fevd, etc).

This way we can reduce copy-pasting. Question is how far we go with this - I guess up to bv_mn.

The auto_psi() function in the file 10_bvar.R, line 176, fit an AR(lags). However, the replication files of Giannone et al. (2015), fit only an AR(1). Wouldn't it be a good option to parametrize also the order of the AR model in the main function? Thanks

as.mcmc.bvar() uses prep_data() to provide wither parameters or variables. In case variables are requested via name we are missing x[["variables"]] in the function.

as.mcmc(x, vars_response = "CPIAUCSL")
Error in get_var_set(vars_response, M = x[["meta"]][["M"]]) : 
  argument "variables" is missing, with no default

Right now we average over T, in the future we should provide them per horizon. Then it makes sense to offer some plotting methods (e.g. stacked bars).

When bvar() is run with problematic data it fails without any usable information - the message is just: Error in value[[3L]](cond) : unused argument (cond).

This absolutely needs to fail more gracefully as it would probably occur frequently.

Stick to base plots for now.

Plots for:

• Traces
• Densities
• IRF
• FEVD
• Forecasts

At the very least plots for prediction should include some of the time series itself.

Also we might add an option for shaded areas vs. lines. Note that there might be more than 3 quants?

It makes sense to work on the interface to coda and facilitate it's use.

When conf_bands is set to 0.5, i.e. no bands the plotting methods for fcasts and irfs fail.

Thanks,

Please also write references in the description field of the description
file in the form
authors (year) doi:...
authors (year) arXiv:...
authors (year, ISBN:...)
with no space after 'doi:', 'arXiv:' and angle brackets for auto-linking.

\dontrun{} is supposed to be used for examples which should not be
called by the user. Please replace \dontrun{} with \donttest{} or unwrap
the examples if they are executable in < 5 sec.

Please fix and resubmit.

Best,
Martina Schmirl

Arias et al. (2018), https://doi.org/10.3982/ECTA14468, provide an algorithm for identification via combined zero- (i.e. a no-change expectation instead of no expectation) and sign-restrictions. After implementing the algorithm it should fit rather well into R/62_sign_restr.R.

Added as of 5ff4b79, some additional info would be nice.

Let users optionally skip the calls to bv_lambda and bv_alpha and do it internally.