BioJulia / Kmers.jl

In development: Kmer types and methods for julia
MIT License
21 stars 7 forks source link

Add docstrings and update docs #30

Closed cjprybol closed 1 year ago

cjprybol commented 1 year ago

Hi,

Thanks for developing this package! Just wanted to help update the documentation a bit, both to help clarify some things for myself but also hopefully for others.

cjprybol commented 1 year ago

Thanks for the review and thoughtful feedback!

I wasn't able to get the StableRNG to work, seems like there are some missing functions that we'd need to implement to pull it off?

julia> rand(StableRNG(1), Kmer{DNAAlphabet{2}, 3})
ERROR: ArgumentError: Sampler for this object is not defined
Stacktrace:
 [1] Random.Sampler(#unused#::Type{StableRNGs.LehmerRNG}, sp::Random.SamplerType{DNAKmer{3, N} where N}, #unused#::Val{1})
   @ Random /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Random/src/Random.jl:145
 [2] Random.Sampler(rng::StableRNGs.LehmerRNG, x::Random.SamplerType{DNAKmer{3, N} where N}, r::Val{1})
   @ Random /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Random/src/Random.jl:139
 [3] rand(rng::StableRNGs.LehmerRNG, X::Random.SamplerType{DNAKmer{3, N} where N})
   @ Random /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Random/src/Random.jl:253
 [4] rand(rng::StableRNGs.LehmerRNG, ::Type{DNAKmer{3, N} where N})
   @ Random /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Random/src/Random.jl:256
 [5] top-level scope
   @ REPL[14]:1

Love the idea in general, and agree it would be worth adding to the doctest (and the regular tests), but adding more functions here (and possibly also in Base.Random?) feels like more than I'm ready to bite off with this pull request.

Instead, I simplified the example in the docstring by removing the random seed setting but also removed the jldoctest flag so it hopefully won't break tests across versions.

Let me know if you think that's a fair compromise if you have another idea for how to handle the RNG for this PR

kescobo commented 1 year ago

but that's not working for me at the moment...

Seems I misunderstood how this works - seed!(rng, 1) sets the seed for rng, not the global RNG. So new methods will indeed be needed.

Have you looked at the source of all of these test failures?

cjprybol commented 1 year ago

@kescobo hadn't looked at the test errors until you asked, but see this issue for what I found https://github.com/BioJulia/Kmers.jl/issues/33

The errors seem unrelated to these changes. Happy to iron them out in a different PR after making a decision on that issue!

kescobo commented 1 year ago

Agreed - You've got a green check on docs, and that's what matters for this PR. Thanks again for the contribution!