-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Continuing development of ZIPLN models #118
Conversation
…or general family of network
…etwork for now... propagte to all PLNs later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the 3 small comments:
- inception_cov to constrain the shape of the covariance matrix in the inception model
- the remark about lapply / future.lapply
- number of parameters for ZIPLNnetwork (the extra p is indeed present in PLNnetwork)
It's all good for me.
R/PLNnetwork.R
Outdated
@@ -122,5 +122,6 @@ PLNnetwork_param <- function( | |||
variance = TRUE , | |||
config_post = config_pst , | |||
config_optim = config_opt , | |||
### TODO CHECK: Why two inceptive model (cov and not ?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced in ctrapnell's PR. Allows the user to:
- specify a custom inception with
inception
- constrain the shape
inception_cov
of the covariance in the (yet-to-be fitted) inception model used to initialize members of the family among full (default, same as before), diagonal, spherical
Useful for large dataset as the "full" model used for initialization (corresponding to$\lambda = 0$ can take a long time to fit.
inception_cov
is ignored isinception
is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted, thx.
cat("\nStability Selection for PLNnetwork: ") | ||
cat("\nsubsampling: ") | ||
|
||
stabs_out <- future.apply::future_lapply(subsamples, function(subsample) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stability_selection()
uses:
future::lapply()
for PLNnetwork butlapply()
for ZIPLNnetwork
Should we switch tolapply()
everywhere ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used lapply
in place of future_lapply
for debugging (the later suppress the output which is not very convenient for debuging). For the moment, I suggest we use future_lapply for all parallel computing, and we will update this particular instance once we will have our complete rethinking of all parallel computing stuff.
R/ZIPLNfit-class.R
Outdated
#' @field nb_param number of parameters in the current PLN model | ||
nb_param = function() { | ||
res <- self$p * self$d + (sum(private$Omega != 0) - self$p)/2L + | ||
res <- self$p * self$d + self$n_edges + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this formula, we don't count diagonal coefficients of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now applies to
Lines 804 to 806 in 36123fd
nb_param_pln = function() { | |
as.integer(self$p * self$d + self$n_edges) | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good for me (and I should indeed have looked at it sooner...)
Still missing
Note that I won't add postTreatment yet zo ZIPLN since I think we need to rethink a bit all the psotTreatment made on PLN-lie objects(in particular, all the variance estimators...)