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

Julia 1.0 Compatibility #41

Merged
merged 37 commits into from
Aug 23, 2018
Merged

Conversation

dcjones
Copy link
Member

@dcjones dcjones commented Aug 13, 2018

This was an ordeal, and ended up being a huge patch, but should bring sequences up to date with julia 1.0 changes.

Needs a couple other PRs to work:

@TransGirlCodes
Copy link
Member

Thanks for taking the time to do this @dcjones! I'll merge when the other two PRs are in 👍

REQUIRE Outdated
IntervalTrees
Twiddle
DataStructures
Nullables
Copy link
Member

Choose a reason for hiding this comment

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

It is okay to have this now but I think that in general we should not depend on deprecated/removed features. We may need a second round to review our APIs.

Copy link
Member

Choose a reason for hiding this comment

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

Nullables are only used to store some optional fields in the FASTA and 2bit parsers, I can change this to the new Union{T, Nothing} style this evening whilst I'm waiting on the Twiddle updates to make it into METADATA.

@TransGirlCodes
Copy link
Member

@dcjones To take this furthur and get CI passes I'm currently waiting on YAML, the version 1.0 changes PR is waiting on a Codecs release, which is here: JuliaLang/METADATA.jl#16489, but as it's not on attobot it's not moving very far. Can you head to the PR and usher them along a bit, or turn on attobot for Codecs and YAML? :)

@codecov
Copy link

codecov bot commented Aug 18, 2018

Codecov Report

❗ No coverage uploaded for pull request base (version-1.0@f16d2e4). Click here to learn what that means.
The diff coverage is 96.78%.

Impacted file tree graph

@@              Coverage Diff               @@
##             version-1.0      #41   +/-   ##
==============================================
  Coverage               ?   94.07%           
==============================================
  Files                  ?       40           
  Lines                  ?     1925           
  Branches               ?        0           
==============================================
  Hits                   ?     1811           
  Misses                 ?      114           
  Partials               ?        0
Impacted Files Coverage Δ
src/bioseq/site_counting/count_sites_bitpar.jl 100% <ø> (ø)
src/bioseq/operators.jl 93.87% <ø> (ø)
src/bioseq/bioseq.jl 100% <ø> (ø)
src/bioseq/hash.jl 100% <ø> (ø)
src/nmask.jl 100% <ø> (ø)
src/bioseq/site_counting/site_counting.jl 100% <100%> (ø)
src/fastq/record.jl 90.74% <100%> (ø)
src/twobit/reader.jl 91.83% <100%> (ø)
src/bioseq/randseq.jl 92.3% <100%> (ø)
src/bioseq/indexing.jl 98.11% <100%> (ø)
... and 28 more

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 f16d2e4...8c5b6b5. Read the comment docs.

@TransGirlCodes
Copy link
Member

TransGirlCodes commented Aug 18, 2018

@dcjones Ok I've made a series of commits trying to get tests to pass. I've got them passing for 1.0, but curiously not for 0.7. What's making me tear my hair out is julia complains "ERROR: LoadError: LoadError: LoadError: syntax: invalid variable expression in "where"". Whilst pointing to "in expression starting at /home/travis/build/BioJulia/BioSequences.jl/src/bioseq/constructors.jl:65". At which is a constructor, which does not make use of the "where" expression anywhere in it's definition. What do you think?

@jgreener64
Copy link
Member

So I had a go and could reproduce your issue @benjward. A MWE without imports that works on v0.7 but not on v1.0 is:

abstract type Alphabet end
abstract type Sequence end

struct DNAAlphabet{n} <: Alphabet end

mutable struct BioSequence{A<:Alphabet} <: Sequence
    data::Vector{UInt64}  # encoded character sequence data
    part::UnitRange{Int}  # interval within `data` defining the (sub)sequence
    shared::Bool          # true if and only if `data` is shared between sequences

    function BioSequence{A}(data::Vector{UInt64},
                            part::UnitRange{Int},
                            shared::Bool) where A
        return new(data, part, shared)
    end
end

function BioSequence{DNAAlphabet{4}}(seq::BioSequence{DNAAlphabet{2}}) # errors here
    newseq = BioSequence{vim}(length(seq))
    #for (i, x) in enumerate(seq)
    #    unsafe_setindex!(newseq, x, i)
    #end
    return newseq
end

It seems that if you replace the offending line with

function BioSequence{T}(seq::BioSequence{DNAAlphabet{2}}) where {T<:DNAAlphabet{4}}

then it seems to work, which is strange. I made a PR to implement this across this file and tests seem to pass on both v0.7 and v1.0.

Feel free to use it if you like, but I'm wondering if this is a Julia issue?

@TransGirlCodes
Copy link
Member

Thanks @jgreener64! You're option made me think of another alternative, which also passes both of 0.7 and 1.0!

abstract type Alphabet end
abstract type Sequence end

struct DNAAlphabet{n} <: Alphabet end

mutable struct BioSequence{A<:Alphabet} <: Sequence
    data::Vector{UInt64}  # encoded character sequence data
    part::UnitRange{Int}  # interval within `data` defining the (sub)sequence
    shared::Bool          # true if and only if `data` is shared between sequences

    function BioSequence{A}(data::Vector{UInt64},
                            part::UnitRange{Int},
                            shared::Bool) where A
        return new(data, part, shared)
    end
end

function (::Type{BioSequence{DNAAlphabet{4}}})(seq::BioSequence{DNAAlphabet{2}})
    newseq = BioSequence{DNAAlphabet{4}}(length(seq))
    for (i, x) in enumerate(seq)
        unsafe_setindex!(newseq, x, i)
    end
    return newseq
end

@TransGirlCodes
Copy link
Member

Merging into the version-1.0 branch now. Release to follow shortly.

@TransGirlCodes TransGirlCodes merged commit 4469449 into BioJulia:version-1.0 Aug 23, 2018
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.

4 participants