Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
szhorvat
force-pushed
the
fix/sample_motifs
branch
from
b84732e to
39b0784
Compare
R/motifs.R
Outdated
| #' sample_motifs(g, 3) | ||
| sample_motifs <- function(graph, size = 3, cut.prob = rep(0, size), | ||
| sample.size = vcount(graph) / 10, sample = NULL) { | ||
| sample.size = ceiling(vcount(graph) / 10), sample = NULL) { |
There was a problem hiding this comment.
this default is getting a bit long. could we make it NULL and do the complicated stuff within the function? https://design.tidyverse.org/defaults-short-and-sweet.html#null-default
| expect_equal(m5 / m, c(NA, NA, 0.439985332979302, NA, 0.440288166730411, 0.346938775510204, 0.44159753136382, 0.452054794520548, NaN, 0.323076923076923, NaN, 0.347826086956522, NaN, NaN, NaN, NaN)) | ||
| }) | ||
|
|
||
| test_that("sample_motifs works", { |
There was a problem hiding this comment.
| test_that("sample_motifs works", { | |
| test_that("sample_motifs() works", { |
tests/testthat/test-motifs.R
Outdated
| g <- make_graph(~ A-B-C-A-D-E-F-D-C-F) | ||
| n <- vcount(g) | ||
|
|
||
| mc <- sample_motifs(g) |
There was a problem hiding this comment.
can we name the three mc things differently to make them distinct?
…motifs()` and ensure that the default `sample.size` is integer
krlmlr
force-pushed
the
fix/sample_motifs
branch
from
abf9f8b to
dca5e2f
Compare
|
Thanks! |
| graph, | ||
| size = 3, | ||
| cut.prob = rep(0, size), | ||
| sample.size = NULL, |
There was a problem hiding this comment.
I'm a bit uncomfortable about the default being hidden behind NULL, as the value is actually very relevant during usage, and this default value is completely arbitrary.
IMO the ideal solution would be to force users to provide either sample.size or sample explicitly instead of relying on the default. However, that is a breaking change not suitable for this release.
There was a problem hiding this comment.
could you please open an issue for this?
There was a problem hiding this comment.
Actually, the rationale is documented, the defaults are documented, and this will end up having a very low priority for a long time.
https://design.tidyverse.org/defaults-short-and-sweet.html#null-default
There was a problem hiding this comment.
We can change the interface to make arguments required, even in a lifecycle-friendly way. I think I misread this as a request to put back the inline default.
We could start with a message that is shown if the argument is omitted but would have been relevant.
|
This pull request failed to merge: some required checks failed. After you have resolved the problem, you should remove the Failed checks: Sanitizer |
|
The CI failure is related to rgl. I think Aviator should be overridden and this should be merged. Either there's a real memory leak in rgl (in which case this should be reported to them, but it's too much work to create a minimal example right now), or it's a LeakSanitizer false positive (in which case we need to add a suppression, which is also annoying and time consuming). |
3 participants
@krlmlr Please use this fix instead of the one in #1565, which doesn't seem correct. This also fixes a bad default value for the
sample.sizeparameter. Recent igraph errors for non-integer values.