Skip to content

Preallocate lists and vectors before iteration loops#341

Open
srishtiii28 wants to merge 3 commits into
mlverse:mainfrom
srishtiii28:preallocate-lists-vectors
Open

Preallocate lists and vectors before iteration loops#341
srishtiii28 wants to merge 3 commits into
mlverse:mainfrom
srishtiii28:preallocate-lists-vectors

Conversation

@srishtiii28

Copy link
Copy Markdown
Contributor

Closes #335

Lists that grow one element at a time reallocate on every iteration. When the final length is known before the loop, they're now allocated with vector("list", n) and assigned by index.

folder_make_dataset() and the cityscapes image collection now gather into a preallocated list and unlist() once, instead of calling c() per iteration.

@srishtiii28

Copy link
Copy Markdown
Contributor Author

In rfdetr_model$forward(), masks is only preallocated when a mask is supplied. It is passed downstream as if (length(masks) > 0) masks else NULL, so preallocating it unconditionally breaks the default no-mask path, hence changes have been made keeping that in mind.

@cregouby cregouby left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise a very good step in the perfromance improvement direction, thanks !
todo specific cases would benefit from Map() and lapply() usage
todo see inline

Comment thread R/folder-dataset.R

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion readability instead of first allocation then iteration, it seems this use case could make use of the functionnal lapply() that do both at the same time

# Remplacement de la boucle for par lapply
res <- lapply(sort(names(class_to_idx)), function(target_class) {
  class_index <- class_to_idx[[target_class]] 
  target_dir <- fs::path_join(c(directory, target_class))
  
  # we may gather empty vectors that will be removed by unlist at the end
  if (!fs::is_dir(target_dir)) {
    return(list(paths = character(0), indexes = integer(0)))
  }
  
  fnames <- fs::dir_ls(target_dir, recurse = TRUE)
  fnames <- fnames[is_valid_file(fnames)]
  
  list(paths = fnames, indexes = rep(class_index, length(fnames)))
})

list(
  unlist(lapply(res, `[[`, "paths"), use.names = FALSE),
  unlist(lapply(res, `[[`, "indexes"), use.names = FALSE)
)

todo perfromance from that point, it becomes transparent that the code is running n_classes times the fs::dir_ls() what can be is extremely time consuming. I would recommand to make that ls_dir() once and then filter out invalid files, non-classes related files, and so on.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo readability We could make use of Map() function for the double iteration instead of iteration on i and iteration on j :

feat_fuse <- Map(function(f, y) f(y), self$stages_sampling[[i]], x)

Comment thread R/models-rfdetr_detection.R Outdated
Comment on lines 666 to 672
out <- vector("list", length(feats))
for (i in seq_along(feats)) {
feat <- feats[[i]]
m <- nnf_interpolate(mask$unsqueeze(1)$float(), size = feat$shape[3:4])$to(dtype = torch_bool())[1, , ]
out[[length(out) + 1]] <- list(tensors = feat, mask = m)
out[[i]] <- list(tensors = feat, mask = m)
}
out

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo readability Here lapply() is a good candidate as well:

Suggested change
out <- vector("list", length(feats))
for (i in seq_along(feats)) {
feat <- feats[[i]]
m <- nnf_interpolate(mask$unsqueeze(1)$float(), size = feat$shape[3:4])$to(dtype = torch_bool())[1, , ]
out[[length(out) + 1]] <- list(tensors = feat, mask = m)
out[[i]] <- list(tensors = feat, mask = m)
}
out
out <- lapply(feats, function(feat) {
m <- nnf_interpolate(mask$unsqueeze(1)$float(), size = feat$shape[3:4])$to(dtype = torch_bool())[1, , ]
list(tensors = feat, mask = m)
})

Comment on lines -691 to 696
pos <- list()
pos <- vector("list", length(feats))
for (i in seq_along(feats)) {
pos[[i]] <- self[["1"]](feats[[i]]$tensors, align_dim_orders = FALSE)
}
list(feats, pos)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo readability a double lapply() good candidate
suggestion

    if (length(feats) > 0 && is.list(feats[[1]]) && !is.null(feats[[1]]$tensors)) {
       pos <- lapply(feats, function(f) self[["1"]](f$tensors, align_dim_orders = FALSE)
    } else {
      pos <- lapply(feats, function(f) self[["1"]](f, align_dim_orders = FALSE))
    }
    
    list(feats, pos)

@srishtiii28 srishtiii28 requested a review from cregouby July 12, 2026 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preallocate lists and vectors with the right size to prevent reallocation during iterations

2 participants