ModelOriented/iBreakDown

DALEXverse 0.19.8 release summer 2019

Closed this issue · 5 comments

pbiecek commented

DALEXverse 0.19.8 release summer 2019

Integration

  • readability: vignettes
  • readability: NEWS
  • readability: DESCRIPTION
  • consistency: pkgdown website
  • consistency: entry at DrWhy.AI webpage

assigned: @pbiecek

Code review

  • consistency: names of functions
  • consistency: names of files
  • consistency: names of variables in functions (local and global)
  • length: functions
  • readability: code (comments, constructions)

assigned: @maksymiuks

Feature review

  • readability: documentation (title, description, details)
  • readability: examples (relevant, complete, with comments)
  • reproducibility: tests (code coverage)
  • links to functions: \code

assigned: @kasiapekala

maksymiuks commented
hbaniecki commented

remove caret from suggest

hbaniecki commented

change URL in DESCRIPTION to github + add demo.gif

kasiapekala commented

tests
https://github.com/ModelOriented/iBreakDown/blob/master/tests/testthat/test_break_down.R
adding break_down with interactions here, along the bd_rf, and similarly testing for the break_down class, would increase the code coverage

break_down, local_interactions, local_attributions
https://github.com/ModelOriented/iBreakDown/blob/master/R/local_attributions.R#L5
https://github.com/ModelOriented/iBreakDown/blob/master/R/local_interactions.R#L6
I was going to suggest using \code{\link[breakDown]{break_down}}, but couldn't find function break_down() in package breakDown.

https://github.com/ModelOriented/iBreakDown/blob/master/R/local_interactions.R#L14
could be changed to: "By default it is equal to '1'. The larger the number is, the more frequently interactions will be presented in explanations."

break_down_uncertainty
https://github.com/ModelOriented/iBreakDown/blob/master/R/break_down_uncertainty.R#
calles -> calls
calculated -> calculates

https://github.com/ModelOriented/iBreakDown/blob/master/R/break_down_uncertainty.R#L6
without "by"

describe
https://github.com/ModelOriented/iBreakDown/blob/master/R/describe_breakdown.R#L12
variables counteracting with each other -> variables counteracting each other

https://github.com/ModelOriented/iBreakDown/blob/master/R/describe_breakdown.R#L14
choosen -> chosen
defalut -> default
dicribes -> describes
which chooses three most significant variables -> maybe would be better to avoid repeating the verb and write "which selects three most significant variables"

https://github.com/ModelOriented/iBreakDown/blob/master/R/describe_breakdown.R#L19
from the description of the short_description parameter and from this line is not obvious for me whether the short description is returned instead of the longer one or somehow along it

https://github.com/ModelOriented/iBreakDown/blob/master/R/describe_breakdown.R#L14
https://github.com/ModelOriented/iBreakDown/blob/master/R/describe_breakdown.R#L22
argumentation_mode is mentioned twice (in slightly contradictory ways), but I don't see it in describe() parameters

https://github.com/ModelOriented/iBreakDown/blob/master/R/describe_breakdown.R#L3
description -> descriptions
what enchaces -> that enchace

plots and prints
https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down_uncertainty.R
description could elaborate more on what exactly is plotted, since the break_down_uncertainty object is a little bit more complex than break_down object

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down.R#L4
missing \code links

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down.R#L6
https://github.com/ModelOriented/iBreakDown/blob/master/R/plotD3_shap.R#L6
https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down_uncertainty.R#L3
repeated word 'model'
(the same for print.break_down)

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down.R#L9
(the same for plotD3_break_down.R and plotD3_shap.R)
By deafult NA therefore will be extracted from -> By default NA, therefore it will be extracted from
But can be set to some constants, usefull if these plots are used for comparisons. -> But it can be set to some constants, useful if these plots are to be used for comparisons.

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down.R#L10
if TRUE, variable contributions will be added to the plot

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down.R#L15
(the same for plotD3_break_down.R, plotD3_shap.R, print.break_down)
function that is to used for -> function to be used for
Second sentence could be changed for:
This should be \code{\link{signif}} which keeps a specified number of significant digits or \code{\link{round}} (which is default) to have the same precision for all components.

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down.R#L11
how much labels sholud be shifted right as a fraction of range. By default 0.05
-> number describing how much labels should be shifted to the right, as a fraction of range. By default equal to 0.05.

https://github.com/ModelOriented/iBreakDown/blob/master/R/plot_break_down_uncertainty.R#L5
I think words 'logical' and 'default' are not necessary

https://github.com/ModelOriented/iBreakDown/blob/master/R/plotD3_break_down.R#L5
missing \code links

https://github.com/ModelOriented/iBreakDown/blob/master/R/plotD3_break_down.R#L18
(the same for plotD3_shap.R)
if TRUE, the height of plot scales with window size

https://github.com/ModelOriented/iBreakDown/blob/master/R/plotD3_break_down.R#L21
(the same for plotD3_shap.R)
By default NA therfore will choose DrWhy colors -> If NA (default), DrWhy colors are used.

https://github.com/ModelOriented/iBreakDown/blob/master/R/plotD3_shap.R#L4
\code{\link{shap}}

  • ❤️1
AdamIzdebski commented

remove caret from suggest

It is needed for building vignettes