Using vapply() and seq_*()

ocbe-uio / cellmigRation

An R package for tracking cells and analyzing their trajectories

https://bioconductor.org/packages/cellmigRation/

0 stars 1 forks source link

Using vapply() and seq_*() #37

Closed wleoncio closed 3 years ago

wleoncio commented 4 years ago

Source: https://github.com/Bioconductor/Contributions/issues/1618#issuecomment-696258765

[x] REQUIRED: Avoid sapply(); use vapply() where possible.
[x] REQUIRED: Avoid 1:...; use seq_len() or seq_along() where possible.

Please remember to add (#37) to the description of the commit fixing this issue.

wleoncio commented 4 years ago

Other connected PRs: #48, #49, #50, #51, #52, #53, #54, #55, #56. Mentioned here because the limit of linked PRs on the right panel has been reached.

osorensen commented 4 years ago

I have now remove all instances of sapply(), and also added seq_len and seq_along in the vicinity of sapply. In addition to the PRs mentioned above, there is a whole lot of merges directly to the develop branch. I have inspected the code and run it with the example data, where this exists, but there is of course a chance that I have introduced some bugs. When unit tests are in place for all functions (cf. #36 ), it will be easy to find any commits which might break the code.

wleoncio commented 4 years ago

Super work, @osorensen! I did find one leftover sapply(1: here:

https://github.com/ocbe-uio/cellmigRation/blob/32dfe93fe148d5d87fcad5321a8c154a3b2c72e1/R/all_functions.R#L611

wleoncio commented 4 years ago

For the record, there are currently still 363 instances of 1:x left to be potentially replaced with seq_len(x) or seq_along(x):

As a general rule, seq_len(x) is the usual solution when x is an integer; if x is a vector, seq_along(x) is what you're probably looking for.

osorensen commented 4 years ago

@wleoncio , the leftover sapply you mentioned above is related to this PR #41 , and will be fixed once this one is merged. I think it is fine, so will merge it right away.

wleoncio commented 4 years ago

@wleoncio , the leftover sapply you mentioned above is related to this PR #41 , and will be fixed once this one is merged. I think it is fine, so will merge it right away.

Oh, right, I forgot that one's still open! :grimacing:

osorensen commented 4 years ago

Regaring seq_len, Alvaro is aware that I don't have time for these, so I assume someone else will fix these.

wleoncio commented 4 years ago

This issue has been split into #59, #60 and #61.

wleoncio commented 3 years ago

Fixed on f38498fdda3554d5ef33613d1b5ee4970b94bdc2.