-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix namespace #418
fix namespace #418
Conversation
@clarkliming , number changed a lot, are they ok? |
🧪 Code Coverage Summary
Diff against main
Results for commit: 2f83c76b4b232b4ffa905938041c272025df784d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Test Performance Difference
Results for commit 2f83c76b4b232b4ffa905938041c272025df784d ♻️ This comment has been updated with latest results. |
@shajoezhu in #385 this PR changes a lot of the snapshots. Here it is reverted back. previous changes should not lead to successful github check actions. Do you have any idea why it passed? |
3 10 (7.5%) 16 (11.9%) 13 (9.8%) | ||
4 12 (9.0%) 11 (8.2%) 13 (9.8%) | ||
Any 55 (41.0%) 56 (41.8%) 62 (47.0%) | ||
1 18 (13.4%) 16 (12.0%) 18 (13.6%) |
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.
why is this happening?
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.
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.
there are three instances of sample
in the syn_test_data
that generate the data for testing for adlb (here
Line 173 in 9f3d0ce
sample(x = c("LAST", "REPLICATED", "SINGLE"), size = n(), replace = TRUE, prob = c(0.3, 0.6, 0.1)), |
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.
seed are already set and this should not change anything
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 think the seed should be set in the test itself if something is generated anew there
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.
but adlb is generated beforehand is it?
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.
seeds are consumed by first usage of the sample
e.g.
> library(dplyr)
>
> set.seed(840)
> m2 <- mtcars %>%
+ mutate(s1 = sample(c("A", "B"), nrow(.env$mtcars), prob= c(0.5, 0.5), replace = TRUE)) %>%
+ mutate(s2 = sample(c("A", "B"), nrow(.env$mtcars), prob= c(0.5, 0.5), replace = TRUE))
>
> identical(m2$s1, m2$s2)
[1] FALSE
can be fixed by the ugly:
> m3 <- mtcars %>%
+ mutate(s1 = {
+ set.seed(840)
+ sample(c("A", "B"), nrow(.env$mtcars), prob= c(0.5, 0.5), replace = TRUE)
+ }) %>%
+ mutate(s2 = {
+ set.seed(840)
+ sample(c("A", "B"), nrow(.env$mtcars), prob= c(0.5, 0.5), replace = TRUE)
+ })
>
> identical(m3$s1, m3$s2)
[1] TRUE
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 always thought it was scope-wise, i.e. valid for each "block" of code. This is not a big problem in other stuff I guess because we always have only one sample per testing block. I think we need to fix the seed for every call then!! Nice catch Benoit
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.
Lgtm anyway, if we can guarantee this is not changing in the next PR due to data reinitialization. It is necessary though to do testthat::snapshot_accept(<FILE>)
before all to accept the changes in _snaps and avoid the .new files
Use fix mentioned above if necessary |
I think these new files are also created in #385 . in this PR these new files are removed |
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.
You are right @clarkliming! I think we can eventually fix the sample problem after this
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.
Lgtm! Thanks Emily for the fix and Benoit for catching the problem ;)
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.
Works for me too, clear for merge, thanks a lot
close #417