Closed mwelz closed 2 years ago
Thanks, in general it looks very nice and the class structure is good, but there are some details that need attention.
Naming can be improved:
"p value"
as the column name in the printed output. Accordingly, the component in the "BLP_info"
objects etc. should be called "p_value"
to be consistent with the other components, where you use underscores to separate words (e.g., confidence_interval
).confidence_interval
, but the printed output uses CB
for confidence bounds. Also, "CB lower"
is a rather cumbersome abbreviation for "lower confidence bound" since the words are out of order. I'd suggest to use "Lower CB"
to reflect the correct word order, "CI lower"
to refer to the confidence interval lower bound (which would be more consistent with the naming of the component in the object), or simply "Lower"
. Also the printed information on the confidence level should change accordingly. For example, if you use the latter, the message should be something like Confidence level of confidence interval [Lower, Upper]:
.In the documentation, not all instances of class names are put in quotation marks, and not all function names are followed by parentheses:
GenericML()
where these are missing.These are details, but attention to those details matters if we want people to be confident about using the package.
Thank you for your comments, @aalfons! I have implemented your suggested changes; I ultimately went for "CI lower
etc. as printed name of the confidence bounds.
get_best()