Fix doctests #342

devmotion · 2021-12-23T09:56:39Z

No description provided.

devmotion · 2021-12-23T10:08:25Z

Weird, I ran doctest(MCMCChains; fix=true) locally with Julia 1.7 and everything seems to pass.

rikhuijzer · 2021-12-23T11:33:34Z

How about throwing away some compatibility stuff for Julia version 1.6 and lower? We're helping almost no one with that and just making the lives of the few Turing developers more difficult

EDIT: Also, if someone is still on Julia 1.2, it's very unlikely that that person is gonna install the newest MCMCChains version anyway. Those people can just keep using their old MCMCChains version even after bumping the lower compat bound here.

devmotion · 2021-12-23T12:17:53Z

The main problem is not 1.0. We don't have to do anything special in this package to support it, so I don't think we should drop it.

The main problems are 1) that array printing is different depending on the Julia version, also from 1.6 to 1.6, and 2) the MLJ dependencies for the Rstar tests. Both require fixes even if we would support only >= 1.6.

The XGBoost dependency is annoying (and eg doesn't work on 32bit so we can't run tests there), and hence probably it would be helpful to use some simpler model in the tests (eg in MCMCDiagnosticTools I use Libsvm as well but it also has binary dependencies, so maybe eg naive Bayes would be even better). Additionally, there's a new release of MLJBase that breaks our tests - however, fixing it to an old version breaks our current test setup.

rikhuijzer · 2021-12-23T12:44:16Z

The main problem is not 1.0. We don't have to do anything special in this package to support it, so I don't think we should drop it.

This is special:

MCMCChains.jl/test/runtests.jl

Lines 3 to 8 in e7b3db5

    
           # Activate test environment on older Julia versions 
        
           if VERSION < v"1.2" 
        
               Pkg.activate(@__DIR__) 
        
               Pkg.develop(PackageSpec(path=dirname(@__DIR__))) 
        
               Pkg.instantiate() 
        
           end

MCMCChains.jl/test/runtests.jl

Lines 22 to 27 in e7b3db5

    
           if VERSION >= v"1.3" && VERSION < v"1.7" && Sys.WORD_SIZE == 64 
        
               # run tests related to rstar statistic 
        
               println("Rstar") 
        
               Pkg.add("MLJBase") 
        
               Pkg.add("MLJXGBoostInterface") 
        
               @time include("rstar_tests.jl")

EDIT: Let's move this discussion to the Turing slack. No need for me to spam you here on this PR.

rikhuijzer · 2021-12-23T12:46:53Z

and eg doesn't work on 32bit so we can't run tests there

Also, who runs 32 bit in this day and age? Apple dropped support for 32-bit in OS X Lion in 2011.

devmotion · 2021-12-23T14:30:51Z

These specialization are not the problem here, and at least the second one is still needed even if we drop < 1.6.

32bit tests helped to uncover Int32/Int64 issues.

rikhuijzer · 2021-12-23T15:03:43Z

32bit tests helped to uncover Int32/Int64 issues.

nice okay

codecov · 2021-12-23T21:49:41Z

Codecov Report

Merging #342 (e94f2df) into master (e7b3db5) will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   82.94%   83.09%   +0.15%     
==========================================
  Files          19       19              
  Lines         979      988       +9     
==========================================
+ Hits          812      821       +9     
  Misses        167      167

Impacted Files	Coverage Δ
src/chains.jl	`83.27% <ø> (+0.05%)`	⬆️
src/rstar.jl	`100.00% <ø> (ø)`
src/tables.jl	`100.00% <0.00%> (ø)`
src/summarize.jl	`83.56% <0.00%> (+0.22%)`	⬆️
src/utils.jl	`95.12% <0.00%> (+0.31%)`	⬆️
src/sampling.jl	`60.00% <0.00%> (+4.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7b3db5...e94f2df. Read the comment docs.

devmotion · 2021-12-23T22:19:57Z

I fixed the issues and the annoying binary dependencies by replacing XGBoost with DecisionTree.

There are no additional fixes or workarounds for specific Julia versions needed, and actually now the rstar statistic can be tested with any Julia version and also on 32bit architectures

Doctests are only performed with Julia >= 1.7 and on 64bit which fixes all issues with array printing in different Julia versions (again, not a Julia 1.0 thing but also a problem for Julia 1.6 vs 1.7). IMO doctests really should only be performed with one specific Julia versions due to changes in e.g. print, show, and RNGs (doctest filters can mask outdated documentation since matches in the expected and actual outcome will be removed before comparing them). However, using >= v"1.7" instead of == v"1.7" ensures that we don't silently drop doctests when a new Julia version is released and CI is updated.

rikhuijzer

Looks very good to me. Pure Julia is definitely better than XGBoost with its binary dependency

rikhuijzer · 2021-12-24T09:55:34Z

test/runtests.jl

+    # run tests related to rstar statistic
+    println("Rstar")
+    @time include("rstar_tests.jl")
+
+    # Array printing depends on Julia version and architecture,
+    # therefore we only run doctests with Julia >= 1.7 and on 64bit where `Int === Int64`
+    if VERSION >= v"1.7" && Sys.WORD_SIZE == 64
+        DocMeta.setdocmeta!(
+            MCMCChains,
+            :DocTestSetup,
+            :(using MCMCChains);
+            recursive = true
+        )
+        doctest(MCMCChains)
+    end


Awesome. Great improvement

* Fix doctests * Fix Github action * Ensure that tests and docs use same package versions * Rearrange test dependencies * Use DecisionTree instead of XGBoost * Only run doctests on 64bit architectures

* Remove direct dependency on SpecialFunctions * Fix doctests (#342) (#344) * Fix doctests * Fix Github action * Ensure that tests and docs use same package versions * Rearrange test dependencies * Use DecisionTree instead of XGBoost * Only run doctests on 64bit architectures

Fix doctests

ce492a0

devmotion added 3 commits December 23, 2021 11:10

Fix Github action

28fb33a

Ensure that tests and docs use same package versions

b0217c0

Rearrange test dependencies

1fcd61f

Use DecisionTree instead of XGBoost

90ec4c2

Only run doctests on 64bit architectures

e94f2df

devmotion requested a review from cpfiffer December 23, 2021 22:53

rikhuijzer approved these changes Dec 24, 2021

View reviewed changes

devmotion merged commit 4548b5c into master Dec 24, 2021

delete-merged-branch bot deleted the dw/fix_doctests branch December 24, 2021 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix doctests #342

Fix doctests #342

devmotion commented Dec 23, 2021

devmotion commented Dec 23, 2021

rikhuijzer commented Dec 23, 2021 •

edited

Loading

devmotion commented Dec 23, 2021

rikhuijzer commented Dec 23, 2021 •

edited

Loading

rikhuijzer commented Dec 23, 2021

devmotion commented Dec 23, 2021

rikhuijzer commented Dec 23, 2021

codecov bot commented Dec 23, 2021 •

edited

Loading

devmotion commented Dec 23, 2021

rikhuijzer left a comment

rikhuijzer Dec 24, 2021

Fix doctests #342

Fix doctests #342

Conversation

devmotion commented Dec 23, 2021

devmotion commented Dec 23, 2021

rikhuijzer commented Dec 23, 2021 • edited Loading

devmotion commented Dec 23, 2021

rikhuijzer commented Dec 23, 2021 • edited Loading

rikhuijzer commented Dec 23, 2021

devmotion commented Dec 23, 2021

rikhuijzer commented Dec 23, 2021

codecov bot commented Dec 23, 2021 • edited Loading

Codecov Report

devmotion commented Dec 23, 2021

rikhuijzer left a comment

Choose a reason for hiding this comment

rikhuijzer Dec 24, 2021

Choose a reason for hiding this comment

rikhuijzer commented Dec 23, 2021 •

edited

Loading

rikhuijzer commented Dec 23, 2021 •

edited

Loading

codecov bot commented Dec 23, 2021 •

edited

Loading