Graph trials #13

richardbeare · 2016-09-20T05:16:11Z

Hi,
Here's some changes that I hope support using stabs with a sparse inverse covariance solver. The main thing I changed inside stabs was the way p was computed, if a "graphical=TRUE" flag is set.

I'm interested to see whether you think this is the right track.

I've provided a little vignette which includes my wrapper function for QUIC. Try it out using:

devtools::install_github("richardbeare/stabs", ref="GraphTrialsA", build_vignettes=TRUE)

vignette("stabs_graphs")

The vignette needs QUIC, huge, knitr

…mes so that it doesn't clash with function

coveralls · 2016-09-20T05:29:46Z

Coverage decreased (-0.3%) to 69.495% when pulling a89be6f on richardbeare:GraphTrialsA into 3bbecfa on hofnerb:master.

hofnerb · 2016-09-20T15:46:40Z

Hi,

that looks nice so far. Some questions / remarks:

You do not have separate input (x) and outcome (y) values, do you? All is based on x only, right? Couldn't / shouldn't we change stabsel.matrix such that it allows to specify x only? In this case, we could check if the fitfun is of type graphical.model (add this as a class to the definition of your fitfun). Based on this we could then internally define y <- x and p <- p * (p-1)/2 which would make an additional argument superfluous.
Usually, you should not need to increase B?

Otherwise your proposed changes look quite reasonable, though I did not check the functionality of the fitfun yet.

Please also add short, quick running tests that checks your novel code. You can use testthat if you like, though I didn't make this move for the existing tests yet.

Thx.

… outcome argument. Updated the vignette to match.

hofnerb · 2016-09-21T06:52:21Z

NAMESPACE

@@ -13,7 +13,7 @@ export(stabsel, stabsel.matrix, stabsel.data.frame, stabsel.stabsel,
       selected, selected.stabsel,
       plot.stabsel, print.stabsel, print.stabsel_parameters,
       glmnet.lasso, glmnet.lasso_maxCoef, lars.lasso, lars.stepwise,
-       subsample)
+       subsample, stabs.quic, getLamPath)


please use standard naming convention, i.e., package name followed by type of model, e.g., quic.graphical_model and do not export getLamPath (I do not see that the user needs that.

hofnerb · 2016-09-21T06:52:21Z

R/stabsel.R

@@ -20,6 +20,15 @@ stabsel.matrix <- function(x, y, fitfun = lars.lasso, args.fitfun = list(),

    cll <- match.call()
    p <- ncol(x) ## TODO: what about intercept?
+    if (missing(y)) {


Please do not base this ONLY on a missing y but also on the class of your fitter

hofnerb · 2016-09-21T06:52:21Z

R/stabsel.R

+      ## Must be a graphical model
+      y <- x
+      p <- p * (p-1)/2
+      graphical <- TRUE


Why do you need the option graphical?

btw. this won't work as you never set this to FALSE in the other cases...

hofnerb · 2016-09-21T06:52:21Z

R/stabsel.R

+      graphical <- TRUE
+      if (verbose) {
+        message("No y (outcome) provided - assuming graphical model")
+      }


Remove this message after the changes indicated above

hofnerb · 2016-09-21T06:56:45Z

R/stabsel.R

@@ -49,11 +58,15 @@ stabsel.matrix <- function(x, y, fitfun = lars.lasso, args.fitfun = list(),
    }

    nms <- colnames(x)
+    if (graphical) {


please add a check based on the class of the fitter as above.

hofnerb · 2016-09-21T06:56:45Z

R/stabselgraphical.R

@@ -0,0 +1,67 @@
+#' Create a regularization path - copied from pulsar.


In general I very much appreciate using roxygen, yet, it is not used in this package so far. Thus I would kindly ask you to remove roxygen and use standard Rd files instead.

You can just leave the roxygen markup in place and never run roxygen. I've committed the documentation files that were created so you can ignore it for now, assuming that you intend to go that direction in the future.

hofnerb · 2016-09-21T06:56:45Z

R/stabselgraphical.R

+  })
+
+  ## Not sure if it is better to have more or less than q
+  lamidx <- which.max(qvals >= q)


can it happen that you do not get equality?

hofnerb · 2016-09-21T06:56:45Z

DESCRIPTION

 Depends: R (>= 2.14.0), methods, stats, parallel
 Imports: graphics, grDevices, utils
-Suggests: TH.data, hdi
+Suggests: TH.data, hdi, knitr, QUIC, igraph, testthat


please add QUIC to Enhances

hofnerb · 2016-09-21T06:57:57Z

I've never used the review feature so far. I hope it helps to get things done more easily. Contact me via email if necessary :)

richardbeare · 2016-09-21T10:49:37Z

I've not found the review interface great for discussion, but see how we go.

I've obviously messed up not initialising graphical - too rushed.

I'm not seeing where you set classes for fitter functions - will this be a new feature or do you already use it somewhere.

Appreciate your feedback on this!

hofnerb · 2016-09-21T11:38:46Z

As I cannot push directly to the pull request please find attached two files which implement the class-based solution I was thinking of.

Proposed_Changes.zip

richardbeare · 2016-09-21T11:46:06Z

OK, got it.

PS - not sure this actually changes ease of use etc, over either an
argument or missing y:

Factors to consider - is there ever a non graphical case with missing y? If
so then clearly need the
function class or the switch argument. If not then the alternatives are
somewhat redundant...

User supplied graphical function wrappers will need to set class - can be
documented but is an unusual thing to do for functions.

Anyhow - will do it this way and see how it goes.

On Wed, Sep 21, 2016 at 9:38 PM, Benjamin Hofner notifications@github.com
wrote:

As I cannot push directly to the pull request please find attached two
files which implement the class-based solution I was thinking of.

Proposed_Changes.zip
https://github.com/hofnerb/stabs/files/484873/Proposed_Changes.zip

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#13 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAvooZSpBBFbrwdSLC--9ysyE7YVkU3Xks5qsRdGgaJpZM4KBO2u
.

hofnerb · 2016-09-21T11:59:43Z

Well, I do not want to clutter the interface with an argument hardly ever needed. We could also check the function names, yet, this would not allow user specified functions at all. Only checking the missing y is somewhat error prone (and we do not know yet if someone else might have similar problems as well). I would not solely rely on that. So using the class of the function is no burden for a standard user (who uses simply the implemented functions) and little burden for someone wanting to implement new functions for graphical models. If you have a better idea please feel free to go ahead. You could also provide a new interface similar to that of mboost::stabsel which doesn't work based on matrizes but based on function arguments. However, I think this is unnecessarily complex as well.

Just one more idea:

We could check if missing(y) and if so, if also inherits(fitfun, "graphical.model"). If both is true, proceed as currently. If not, set the class of fitfun (for later checks) and issue a warning (!). This would allow a perfect solution (without unnecessary messages and a safe behavior) with pre-defined functions such as yours and also an easy(er) solution for new functions.

richardbeare added 6 commits Sep 14, 2016

turn off colnames for inverse covariance

8334f54

added argument for graphical case and changed names variable to outna…

5d8cf83

…mes so that it doesn't clash with function

names for graphical case

6d25205

removed checks in name assignment

6d31c5c

adding vignette

cec426a

tested vignette

Loading status checks…

a89be6f

richardbeare added 6 commits Sep 20, 2016

Removed graphical argument and relied instead on checking for missing…

3dc3138

… outcome argument. Updated the vignette to match.

changed warning to a message

8b0a399

added graphical functions

e90155c

documentation files using roxygen2

ff42087

attributed getLamPath to pulsar

f33968b

basic testthat regression test added

Loading status checks…

7063899

hofnerb reviewed Sep 21, 2016

View changes

hofnerb requested changes Sep 21, 2016

View changes

hofnerb closed this Sep 22, 2016

hofnerb / stabs

Graph trials #13

Graph trials #13

richardbeare commented Sep 20, 2016

coveralls commented Sep 20, 2016

hofnerb commented Sep 20, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

hofnerb commented Sep 21, 2016

richardbeare commented Sep 21, 2016

hofnerb commented Sep 21, 2016

richardbeare commented Sep 21, 2016

hofnerb commented Sep 21, 2016

		@@ -0,0 +1,67 @@
		#' Create a regularization path - copied from pulsar.

hofnerb / stabs

Join GitHub today

Graph trials #13

Graph trials #13

Conversation

richardbeare commented Sep 20, 2016

coveralls commented Sep 20, 2016

hofnerb commented Sep 20, 2016

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

richardbeare Sep 21, 2016 Author Contributor

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

This comment has been minimized.

hofnerb Sep 21, 2016 Owner

hofnerb commented Sep 21, 2016

richardbeare commented Sep 21, 2016

hofnerb commented Sep 21, 2016

richardbeare commented Sep 21, 2016

hofnerb commented Sep 21, 2016

Essential cookies

Always active

Analytics cookies

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

richardbeare Sep 21, 2016
Author Contributor

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner

hofnerb Sep 21, 2016
Owner