Skip to content
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

Investigate test failures #441

Merged
merged 9 commits into from
Sep 21, 2023
Merged

Conversation

sathvikbhagavan
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #441 (8acff99) into master (ddca4f8) will decrease coverage by 6.22%.
Report is 29 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   79.89%   73.68%   -6.22%     
==========================================
  Files          16       22       +6     
  Lines        2567     2930     +363     
==========================================
+ Hits         2051     2159     +108     
- Misses        516      771     +255     
Files Changed Coverage Δ
lib/SurrogatesMOE/src/SurrogatesMOE.jl 0.00% <0.00%> (ø)
src/Radials.jl 84.48% <ø> (+1.72%) ⬆️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sathvikbhagavan sathvikbhagavan force-pushed the sb/fix_tests branch 2 times, most recently from 4444321 to a9f86e0 Compare September 20, 2023 08:55
This is to test whether it passes CI with an earlier version or not
@sathvikbhagavan sathvikbhagavan force-pushed the sb/fix_tests branch 2 times, most recently from 3cb750c to a4139bb Compare September 20, 2023 12:13
This is to ensure all testsets run even if one fails as they are independent
@sathvikbhagavan
Copy link
Member Author

@ChrisRackauckas

I investigated a bit about the Surrogates test failures and wanted to summarize them here:

  1. Tests in SurrogatesMOE fail spuriously -
    i. Successful run - https://github.com/SciML/Surrogates.jl/actions/runs/6248364164?pr=441
    ii. Unsuccessful run - https://github.com/SciML/Surrogates.jl/actions/runs/6248569007?pr=441
    This happens when one of the clusters do not have any test points which is not handled (not 100% sure)

  2. using QuasiMonteCarlo 0.2.19 (latest of 0.2) fails tests in GEKPLS tests. But using 0.2.16 works and passes all tests. The reason explained in Test master #420

  3. Cannot bump QuasiMonteCarlo to 0.3 as for some reason https://github.com/SciML/QuasiMonteCarlo.jl/blob/master/src/Section.jl is commented and it complains about SectionSample not defined.

  4. I had to bump the compat of ExtendableSparse to 1 as I get this error
    ERROR: MethodError: \(::Symmetric{Float64, ExtendableSparseMatrix{Float64, Int64}}, ::Vector{Float64}) is ambiguous with 0.6 in julia 1.9.
    After bumping, it works.

So, I think (1) can be fixed but I am not sure what's happening with QuasiMonteCarlo currently.

@ChrisRackauckas
Copy link
Member

Okay, we should just bound QMC to 0.2.16, set the RNG seed for 1. Let's see how far that takes us.

Comment on lines 2 to 3
using Random
Random.seed!(100)
Copy link
Member

Choose a reason for hiding this comment

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

Use StableRNGs.jl

@ChrisRackauckas ChrisRackauckas marked this pull request as ready for review September 21, 2023 04:25
@sathvikbhagavan
Copy link
Member Author

I think the reason MOE tests were failing spuriously as division between train and test data was not same each run because of using BitArray(undef, n) in https://github.com/SciML/Surrogates.jl/blob/master/lib/SurrogatesMOE/src/SurrogatesMOE.jl#L144
as all elements are not false all the time (we want all of them to be false)

This is fixed in 6848450

julia> for _ in 1:10
              @info sum(BitArray(undef, 150))
              end
[ Info: 26
[ Info: 49
[ Info: 4
[ Info: 3
[ Info: 25
[ Info: 49
[ Info: 7
[ Info: 3
[ Info: 51
[ Info: 48

@ChrisRackauckas ChrisRackauckas merged commit 1ee0a5c into SciML:master Sep 21, 2023
4 of 7 checks passed
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.

2 participants