Feedback - Githubissues

Does the package provide enough functions to fulfill its goal?
-It certainly fulfills in the sense that it provides some core functions but I would also consider quantile and random number simulation from the discrete phase-type to be core functions and these are not provided.
Description file: Are all other packages that are needed listed in the package DESCRIPTION ? Are all authors included in the description?
-Yes.

Does the package install without problems?
-Yes.
Try to challenge the code with own examples - are the functions robust, or do they break, e.g. let R freeze?
-I haven't be able to make R freeze but for example mean(phase_type(subint_mat = as.matrix(0),init_probs = 0)) gives an error not from this package but a package it uses, and I have to been able get negative numbers from the var function, which shouldn't be possible.
phase_type(type = 'T_MRCA', subint_mat = as.matrix(0), init_probs = -99) also yields an error message from deeper within R.
I am able to give a negative mutation rate.
The output of disc_phase_type(subint_mat = as.matrix(0)) is a phase_type object, I'm guessing this is not intended.
qphtype(.9975,phase_type(type= 'T_Total',n =100)) gives an error from uniroot.
Does the package have (and pass) its own testing?
-Yes, it passes its own testing.
Level and appropriateness of advanced programming techniques, in particular S3 ob- jects: have advanced programming techniques been used, and if yes, where, and do you consider this appropriate?
-It would make sense to make more of your functions into S3-methods instead of this if(class(object)=='disc_phase_type')-stuff.

Are all functions correctly and sufficiently documented? Do the help files provide useful examples?
-The example of a user-generated subintensity matrix is not a subintensity matrix.
RameMAndStateSpace, rewardtransformparm and var are not documented sufficiently, and mean is not documented at all.
I find in unclear in for example phase_type which arguments must be provided together i.e. type and n must be provided together, but also when arguments are ignored. For example phase_type(n=3, subint_mat = as.matrix(0), init_probs = -99) ignores the n parameter, and phase_type(type = 'T_Total', n = 3, subint_mat = as.matrix(0), init_probs = -99) ignores both subint_mat and initprobs.
What does the granularity parameter do in rphasetype? Does it make sense that it can be set to for example 99?
In the documentation for sfs I find the formulation "There is no need to supply entire vector if most of its elements are zero." unclear. At which end of the vector are the zeroes added?
Are vignettes supplied with the package? Do the vignettes provide some background to understand what the package is about (unless this is trivial)?
-The vignettes are not accessible from 'Help'. I found it hard to understand what a phase-type distribution is, reading the vignette. The last section where the heading is discrete phase-type distributions, is only about reward transformation and never explains what a discrete phase-type distribution is.

Do the functions check for user mistakes and give warnings when unreasonable arguments are used? (Unfortunately, this is often not the case with the many packages out there!) Are error messages and warnings intelligible?
-There is no check for whether a user-generated subintensity matrix makes sense. A subintensity matrix must satisfy that all diagonal entries are negative, all non-diagonal entries are non-negative and the rowSums must be non-positive. Also there is no check that a user-generated initial distribution is a subprobability vector, i.e. the entries must be non-negative and the sum of the entries must be less than or equal to 1. For example there is no objection to phase_type(subint_mat = matrix(c(9,2,1,4),nrow=2), init_probs = c(-4,-3)), (taking var of this object yields a negative number, this is an alarm bell).

Give an overall valuation of the package. What did you particularly like about it? Is there anything that needs to be improved for a final version?
-I would definitely suggest adding some checks for whether or not the arguments are reasonable(and not provide an example where the arguments are unreasonable!). For phase_type and disc_phase_type you could write something in the documentation about which arguments overrule the others or perhaps make disc_phase_type(n=3, itons=1, tail_stat = T) output two objects, one for the singletons and one for the tail statistic.
Simulation of both the discrete phase-type and continuous phase-type distribution based on Markov Chains is actually very simple to implement, I would suggest doing that. I think you should make your functions S3-methods.

aumath-advancedr2019 / ticphasetype