Merge pull request #741 from njtierney/prune-unused-code-738

Remove unnecessary comments with #browser or #TF1/2

Author: njtierney

R/calculate.R

@@ -390,7 +390,6 @@ calculate_greta_mcmc_list <- function(target,
 }
 
 calculate_list <- function(target, values, nsim, tf_float, env) {
-  ### browser()
   fixed_greta_arrays <- list()
 
   values_exist <- !identical(values, list())
@@ -409,7 +408,6 @@ calculate_list <- function(target, values, nsim, tf_float, env) {
   dag <- dag_class$new(all_greta_arrays, tf_float = tf_float)
 
   stochastic <- !is.null(nsim)
-  ### browser()
   if (stochastic) {
 
     check_if_unsampleable_and_unfixed(fixed_greta_arrays, dag)
@@ -459,7 +457,6 @@ calculate_target_tensor_list <- function(
   nsim
 ) {
   # define the dag and TF graph
-  ### browser()
   # change dag mode to sampling
   dag$mode <- "all_sampling"
 
@@ -498,18 +495,12 @@ calculate_target_tensor_list <- function(
   # approaches (in as_tf_function + generate_log_prob_function)
   dag$define_tf(target_nodes = target_nodes)
 
-  # browser()
   # look up the tf names of the target greta arrays (under sampling)
   # create an object in the environment that's a list of these, and sample that
   target_nodes <- lapply(target, get_node)
   target_names_list <- lapply(target_nodes, dag$tf_name)
 
-  ## TF1/2 OK so the error with Wishart and cholesky is happening here
-  ## I feel as thought this isn't the "problem" per se, but it is where the
-  ## matrix of 1s is returned. So seems to me we first expose the problem here
   target_tensor_list <- lapply(target_names_list, get, envir = tfe)
-  ## It looks like it is getting all_sampling_operation_2 and not
-  ## all_sampling_operation_1
   target_tensor_list_array <- lapply(target_tensor_list, as.array)
 
   return(target_tensor_list_array)

R/checkers.R

@@ -796,9 +796,8 @@ check_n_cores <- function(n_cores,
   n_cores_detected <- future::availableCores()
   allowed_n_cores <- seq_len(n_cores_detected)
 
-  ## TODO check explaining var
-  # check user-provided cores
-  if (!is.null(n_cores) && !n_cores %in% allowed_n_cores) {
+  cores_exceed_available <- !is.null(n_cores) && !n_cores %in% allowed_n_cores
+  if (cores_exceed_available) {
     check_positive_integer(n_cores, "n_cores")
 
     cli::cli_warn(

R/dag_class.R

@@ -21,7 +21,6 @@ dag_class <- R6Class(
     initialize = function(target_greta_arrays,
                           tf_float = "float32",
                           compile = FALSE) {
-      # browser()
       # build the dag
       self$build_dag(target_greta_arrays)
 
@@ -77,34 +76,6 @@ dag_class <- R6Class(
       self$tf_environment$hybrid_data_list <- list()
     },
 
-    # TF1/2 check remove on_graph
-    # built with TF
-    # Not sure if we need this anyore since this information will be handled
-    # by tf_function?
-    # execute an expression on this dag's tensorflow graph, with the correct
-    # float type
-    on_graph = function(expr) {
-      # temporarily pass float type info to options, so it can be accessed by
-      # nodes on definition, without cluncky explicit passing
-      old_float_type <- options()$greta_tf_float
-      old_batch_size <- options()$greta_batch_size
-
-      on.exit(options(
-        greta_tf_float = old_float_type,
-        greta_batch_size = old_batch_size
-      ))
-
-      options(
-        greta_tf_float = self$tf_float,
-        greta_batch_size = self$tf_environment$batch_size
-      )
-
-      # A tf.Graph can be constructed and used directly without a tf.function,
-      # as was required in TensorFlow 1, but this is deprecated and it is
-      # recommended to use a tf.function instead.
-      with(self$tf_graph$as_default(), expr)
-    },
-
     # return a list of nodes connected to those in the target node list
     build_dag = function(greta_array_list) {
       target_node_list <- lapply(greta_array_list, get_node)
@@ -171,7 +142,6 @@ dag_class <- R6Class(
       # if it's an operation, see if it has a distribution (for lkj and
       # wishart) and get mode based on whether the parent has a free state
       if (node_type == "operation") {
-        # browser()
         parent_name <- node$parents[[1]]$unique_name
         parent_stateless <- parent_name %in% stateless_names
         to_sample <- has_distribution(node) & parent_stateless
@@ -282,21 +252,6 @@ dag_class <- R6Class(
         )
       } else {
         shape <- shape(NULL, length(vals))
-        # TF1/2 check?
-        # this `on_graph` part below is probably not needed/wont work because it
-        # is using placeholder and co?
-        #
-        # defining an empty/unknown thing
-        # so in TF2, we might not need to define a free state, we can
-        # define a function that returns these pieces of information
-        # so we will need to define the function relative to the free state
-        # and will not need to define the object
-        # Can we just turn this into a tensor with some arbitrary value?
-        self$on_graph(free_state <- tf$compat$v1$placeholder(
-          dtype = tf_float(),
-          shape = shape
-        ))
-
         # TF1/2 check
         # instead?
         # free_state <- tensorflow::as_tensor(
@@ -322,7 +277,6 @@ dag_class <- R6Class(
       lengths <- lengths(params)
 
       if (length(lengths) > 1) {
-        # args <- self$on_graph(tf$split(free_state, lengths, axis = 1L))
         args <- tf$split(free_state, lengths, axis = 1L)
       } else {
         args <- list(free_state)
@@ -345,9 +299,7 @@ dag_class <- R6Class(
       }
 
       # define all nodes in the environment and on the graph
-      ## HERE
       lapply(target_nodes, function(x){
-        # browser()
         x$define_tf(self)
       })
 

R/node_class.R

@@ -15,7 +15,6 @@ node <- R6Class(
     dim = NA,
     distribution = NULL,
     initialize = function(dim = NULL, value = NULL) {
-      ## browser()
       dim <- dim %||% c(1,1)
 
       # coerce dim to integer
@@ -36,8 +35,6 @@ node <- R6Class(
 
     # recursively register self and family
     register_family = function(dag) {
-      ## TF1/2
-      ## Rename with an explaining variable
       ## TODO add explaining variable
       if (!(self$unique_name %in% names(dag$node_list))) {
 
@@ -75,9 +72,6 @@ node <- R6Class(
       node$remove_child(self)
     },
     list_parents = function(dag) {
-      ## TF1/2
-      ## tf_cholesky
-      ## is there a way here to add some check for cholesky?
       parents <- self$parents
 
       # if this node is being sampled and has a distribution, consider
@@ -200,7 +194,6 @@ node <- R6Class(
           lapply(
             parents[which(!parents_defined)],
             function(x){
-              # browser()
               x$define_tf(dag)
             }
           )

R/node_types.R

@@ -3,7 +3,6 @@ data_node <- R6Class(
   inherit = node,
   public = list(
     initialize = function(data) {
-      ## browser()
       # coerce to an array with 2+ dimensions
       data <- as_2d_array(data)
 
@@ -150,26 +149,7 @@ operation_node <- R6Class(
       mode <- dag$how_to_define(self)
       # if sampling get the distribution constructor and sample this
       if (mode == "sampling") {
-        # browser()
         tensor <- dag$draw_sample(self$distribution)
-
-        # if (has_representation(self, "cholesky")) {
-          # browser()
-          # cholesky_tensor <- tf_chol(tensor)
-          # # cholesky_tf_name <- dag$tf_name(self$representation$cholesky)
-          # cholesky_node <- get_node(representation(self, "cholesky"))
-          # cholesky_tf_name <- dag$tf_name(cholesky_node)
-          # assign(cholesky_tf_name, cholesky_tensor, envir = tfe)
-          ## TF1/2
-          ## This assignment I think is supposed to be passed down to later on
-          ## in the script, as `cholesky_tf_name` gets overwritten
-          # cholesky_tf_name <- dag$tf_name(self)
-          # tf_name <- cholesky_tf_name
-          # tensor <- cholesky_tensor
-          # cholesky_tensor <- tf_chol(tensor)
-          # cholesky_tf_name <- dag$tf_name(self$representation$cholesky)
-          # assign(cholesky_tf_name, cholesky_tensor, envir = dag$tf_environment)
-        # }
       }
 
       if (mode == "forward") {
@@ -188,11 +168,9 @@ operation_node <- R6Class(
         operation <- eval(parse(text = self$operation),
           envir = self$tf_function_env
         )
-        # browser()
         tensor <- do.call(operation, tf_args)
       }
 
-      # browser()
       # assign it in the environment
       assign(tf_name, tensor, envir = dag$tf_environment)
     }
@@ -212,7 +190,6 @@ variable_node <- R6Class(
                           upper = Inf,
                           dim = NULL,
                           free_dim = prod(dim)) {
-      ## browser()
       check_if_lower_upper_numeric(lower, upper)
 
       # replace values of lower and upper with finite values for dimension
@@ -283,33 +260,24 @@ variable_node <- R6Class(
       mode <- dag$how_to_define(self)
 
       if (mode == "sampling") {
-        # browser()
         distrib_node <- self$distribution
 
         if (is.null(distrib_node)) {
           # does it have an anti-representation where it is the cholesky?
             # the antirepresentation of cholesky is chol2symm
-            # if it does, we will take the antirepresentation and get it to `tf` itself
-              # then we need to get the tf_name
+            # if yes, we take antirep and get it to `tf`, then get the tf_name
           chol2symm_ga <- self$anti_representations$chol2symm
           chol2symm_existing <- !is.null(chol2symm_ga)
+
           if (chol2symm_existing) {
+
             chol2symm_node <- get_node(chol2symm_ga)
             chol2symm_name <- dag$tf_name(chol2symm_node)
             chol2symm_tensor <- get(chol2symm_name, envir = dag$tf_environment)
             tensor <- tf_chol(chol2symm_tensor)
+
           }
 
-          #   chol2symm_ga$define_tf(dag)
-          # } else {
-          #
-          # # if the variable has no distribution create a placeholder instead
-          # # (the value must be passed in via values when using simulate)
-          # shape <- to_shape(c(1, self$dim))
-          # # TF1/2 check
-          #   # need to change the placeholder approach here.
-          #   # NT: can we change this to be a tensor of the right shape with 1s?
-          # tensor <- tensorflow::as_tensor(1L, shape = shape, dtype = tf_float())
         } else {
           tensor <- dag$draw_sample(self$distribution)
         }
@@ -432,10 +400,8 @@ distribution_node <- R6Class(
                           discrete = FALSE,
                           multivariate = FALSE,
                           truncatable = TRUE) {
-      ## browser()
       super$initialize(dim)
 
-      ## browser()
       # for all distributions, set name, store dims, and set whether discrete
       self$distribution_name <- name
       self$discrete <- discrete
@@ -462,7 +428,6 @@ distribution_node <- R6Class(
 
     # create a target variable node (unconstrained by default)
     create_target = function(truncation) {
-      ##browser()
       vble(truncation, dim = self$dim)
     },
     list_parents = function(dag) {
@@ -494,7 +459,6 @@ distribution_node <- R6Class(
 
     # create target node, add as a parent, and give it this distribution
     add_target = function(new_target) {
-      ##browser()
       # add as target and as a parent
       self$target <- new_target
       self$add_parent(new_target)
@@ -621,7 +585,6 @@ node_classes_module <- module(
 
 # shorthand for distribution parameter constructors
 distrib <- function(distribution, ...) {
-  ##browser()
   check_tf_version("error")
 
   # get and initialize the distribution, with a default value node

R/probability_distributions.R

@@ -50,7 +50,6 @@ normal_distribution <- R6Class(
   inherit = distribution_node,
   public = list(
     initialize = function(mean, sd, dim, truncation) {
-      ## browser()
       mean <- as.greta_array(mean)
       sd <- as.greta_array(sd)
 
@@ -1051,10 +1050,6 @@ wishart_distribution <- R6Class(
           input_output_cholesky = TRUE
         )
 
-        ## TF1/2
-        ## The issue with getting the cholesky part of the Wishart
-        ## isn't happening here,
-        ## This produces something that looks about right
         chol_draws <- chol_distrib$sample(seed = seed)
 
         # equivalent to (but faster than) tf_chol2symm(tf_transpose(chol_draws))
@@ -1404,7 +1399,6 @@ uniform <- function(min, max, dim = NULL) {
 #' @rdname distributions
 #' @export
 normal <- function(mean, sd, dim = NULL, truncation = c(-Inf, Inf)) {
-  ##browser()
   distrib("normal", mean, sd, dim, truncation)
 }
 

R/unknowns_class.R

@@ -29,8 +29,6 @@ print.unknowns <- function(x, ..., n = 10) {
   # set NA values to ? for printing
   x[is.na(x)] <- " ?"
 
-  # browser()
-
   n_print <- getOption("greta.print_max") %||% n
 
   n_unknowns <- length(x)

R/write-logfiles.R

@@ -37,7 +37,7 @@ write_greta_install_log <- function(path = greta_logfile) {
   <h1>Greta installation logfile</h1>
   <h2>Created: {{sys_date}}</h2>
   <p>Use this logfile to explore potential issues in installation with greta</p>
-  <p>Try opening this in a browser and searching the text for "error" with Cmd/Ctrl+F</p>
+  <p>Try opening this in a HTML browser and searching the text for "error" with Cmd/Ctrl+F</p>
 
   <h2>Miniconda</h2>
 
@@ -142,15 +142,15 @@ sys_get_env <- function(envvar){
 #' Read a greta logfile
 #'
 #' This is a convenience function to facilitate reading logfiles. It opens
-#'   a browser using [utils::browseURL()]. It will search for
+#'   a HTML browser using [utils::browseURL()]. It will search for
 #'   the environment variable "GRETA_INSTALLATION_LOG" or default to
 #'   `tools::R_user_dir("greta")`. To set
 #'   "GRETA_INSTALLATION_LOG" you can use
 #'   `Sys.setenv('GRETA_INSTALLATION_LOG'='path/to/logfile.html')`. Or use
 #'   [greta_set_install_logfile()] to set the path, e.g.,
 #'   `greta_set_install_logfile('path/to/logfile.html')`.
 #'
-#' @return opens a URL in your default browser
+#' @return opens a URL in your default HTML browser.
 #' @export
 open_greta_install_log <- function(){
 

man/open_greta_install_log.Rd

@@ -109,9 +109,3 @@ test_that("Post-MCMC, LKJ distribution stays symmetric, chol remains lower tri",
   expect_upper_tri(calcs$`chol(x)`)
 
 })
-
-
-## TODO
-# further ensure all of the issues in
-## https://github.com/greta-dev/greta/labels/cholesky
-## are resolved in this branch