Neat package, looks like it could be very useful to the end users.
The API documentation could use quite a bit of improvement:
1) API documentation exposes a number of unexported functions - all the "misc" functions starting with a ., such as .derive_detector. It'd be best if these were switched to being internal (using @keywords internal) so they only appear for the developers. Otherwise, it's a little confusing for the user.
2) Repeated references are made to S3 classes - for example, the create_detector(detector_name) function is supposed to return a detector object with an S3 class reflecting the type of detector (e.g. Excalibur). But there aren't actually any S3 classes or methods involved in the package. All of these various functions actually only return lists. I appreciate that S3 objects are essentially just lists with a class attribute, but all the same, a list isn't an S3 object. From reading the docs I'd get the impression that there is an overarching detector class and everything else is a variation on that. But the docs are also inconsistent on this, as they state that each type of detector has its own class (e.g. Pilatus_Detector() returns a "Pilatus detector object" - it doesn't, it returns a list). It looks to me that, since all these various detectors return fundamentally the same structure, you really want a single S3 detector class, for which you could create custom methods (like print.detector(), for example.
3) Overall, the use of many of the functions is opaque to the user. Typically the function documentation simply states the name of the function and gives the title of the help file as the description, without ever really explaining anything about the function. In addition, as far as I can see, none of the function documentation shows any examples of use of specific functions. Examples:
the perform_glm() function. This takes symb_expr and family as arguments. The first is a symbolic description of the linear predictor. how is one of those generated? what does it look like? Do you just mean the usual R formula syntax (Wilkinson notation)? For the family argument, which families are supported? all of the usual ones e.g. could you use gaussian(), Gamma? What can you do with the output? Is it a standard glm class object that's returned?
the plot_angles() function. This is titled Plots NN angles. What does NN stand for? The first input is a ppp_obj. What is a ppp_obj? What can you do with the output?
again, I get the feeling some functions are really meant to be internal rather than user-facing, so would be better left out of the docs using @keywords internal or @noRd
JOSS review issue: openjournals/joss-reviews#2474
Neat package, looks like it could be very useful to the end users.
The API documentation could use quite a bit of improvement:
1) API documentation exposes a number of unexported functions - all the "misc" functions starting with a
., such as.derive_detector. It'd be best if these were switched to being internal (using@keywords internal) so they only appear for the developers. Otherwise, it's a little confusing for the user.2) Repeated references are made to S3 classes - for example, the
create_detector(detector_name)function is supposed to return adetectorobject with an S3 class reflecting the type of detector (e.g.Excalibur). But there aren't actually any S3 classes or methods involved in the package. All of these various functions actually only return lists. I appreciate that S3 objects are essentially just lists with a class attribute, but all the same, a list isn't an S3 object. From reading the docs I'd get the impression that there is an overarchingdetectorclass and everything else is a variation on that. But the docs are also inconsistent on this, as they state that each type of detector has its own class (e.g.Pilatus_Detector()returns a "Pilatus detector object" - it doesn't, it returns a list). It looks to me that, since all these various detectors return fundamentally the same structure, you really want a single S3detectorclass, for which you could create custom methods (likeprint.detector(), for example.3) Overall, the use of many of the functions is opaque to the user. Typically the function documentation simply states the name of the function and gives the title of the help file as the description, without ever really explaining anything about the function. In addition, as far as I can see, none of the function documentation shows any examples of use of specific functions. Examples:
perform_glm()function. This takessymb_exprandfamilyas arguments. The first is asymbolic description of the linear predictor. how is one of those generated? what does it look like? Do you just mean the usual R formula syntax (Wilkinson notation)? For thefamilyargument, which families are supported? all of the usual ones e.g. could you usegaussian(),Gamma? What can you do with the output? Is it a standardglmclass object that's returned?plot_angles()function. This is titledPlots NN angles. What does NN stand for? The first input is appp_obj. What is appp_obj? What can you do with the output?again, I get the feeling some functions are really meant to be internal rather than user-facing, so would be better left out of the docs using
@keywords internalor@noRd