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

Itermaggedon: update everything for 0.7-only #35

Merged
merged 34 commits into from Jun 25, 2018

Conversation

Projects
None yet
3 participants
@iamed2
Contributor

iamed2 commented May 31, 2018

Let's shave some bikes and paint some yaks!

  1. This is already a pretty major change. Should we just ditch chain and product rather than deprecating them?
  2. Are we okay getting rid of the old chain and product? They were worse than the Base versions for a minor benefit (length worked and eltype worked better).
  3. How do we feel about the imap change?
  4. What should we call @something? something is now a function so we should probably not call it that.

Note: 0.7 tests will error until the 0.7-alpha binaries are posted. Nightly tests should pass.

Closes #34
Closes #33 (if we deprecate product) and closes #29 (if we deprecate chain)

iamed2 added some commits May 22, 2018

Deprecate chain in favour of Iterators.flatten
Benchmarks showed that flatten was always several times faster than
chain. We will lose length and eltype but they weren't doing anything
for performance.
end
done(it::Product, state) = state[2] === nothing
@deprecate chain(xss...) Iterators.flatten(xss)

This comment has been minimized.

@ararslan

ararslan May 31, 2018

Member

Don't these behave differently when xss is empty?

This comment has been minimized.

@iamed2

iamed2 May 31, 2018

Contributor

Yes, flatten does this:

julia> collect(Iterators.flatten(()))
ERROR: MethodError: Base.IteratorEltype(::Type{Union{}}) is ambiguous. Candidates:
  Base.IteratorEltype(::Type{#s59} where #s59<:Base.Iterators.Repeated) in Base.Iterators at iterators.jl:726
  Base.IteratorEltype(::Type{#s59} where #s59<:Base.Iterators.PartitionIterator{T}) where T in Base.Iterators at iterators.jl:941
  Base.IteratorEltype(::Type{#s624} where #s624<:Base.Broadcast.Broadcasted) in Base.Broadcast at broadcast.jl:260
  Base.IteratorEltype(::Type{#s59} where #s59<:(Base.Iterators.Rest{I,S} where S)) where I in Base.Iterators at iterators.jl:502
Possible fix, define
  Base.IteratorEltype(::Type{Union{}})

which is...a bug?

This comment has been minimized.

@iamed2

iamed2 May 31, 2018

Contributor

In any case I don't expect these to be direct replacements, which is why I was considering not having deprecations. IterTools.product and Iterators.product had a different iteration order, shape, and 0-element behaviour, but I don't think anyone really needs the IterTools behaviour.

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

Looks like deprecating chain will break Gadfly, so we should see if they can use Iterators.flatten instead.

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

Compose uses both chain and product, and may depend on the ordering for product. We should attempt to update that one first.

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

Text.jl needs a simple fix to use flatten instead of chain.

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

PhyloNetworks.jl may depend on the ordering of product

@ararslan

This comment has been minimized.

Member

ararslan commented Jun 1, 2018

Nice analysis of packages! Is it the empty case for chain that will break Gadfly when using flatten?

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

BasisMatrices.jl uses product and depends on order but we can switch it to Iterators.product order by swapping some indices

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

@ararslan In Gadfly they actually specialize a method on IterTools.Chain, so that's the main thing that needs to be fixed

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

I think BioSequences.jl doesn't rely on the ordering of product but it's probably good to check.

@iamed2 iamed2 closed this Jun 1, 2018

@iamed2 iamed2 reopened this Jun 1, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jun 1, 2018

Codecov Report

Merging #35 into master will increase coverage by 7.16%.
The diff coverage is 94.8%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #35      +/-   ##
=========================================
+ Coverage   88.64%   95.8%   +7.16%     
=========================================
  Files           2       1       -1     
  Lines         317     143     -174     
=========================================
- Hits          281     137     -144     
+ Misses         36       6      -30
Impacted Files Coverage Δ
src/IterTools.jl 95.8% <94.8%> (+7.34%) ⬆️

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 a2b4ed3...968abc9. Read the comment docs.

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 1, 2018

Package Updates

  • Gadfly
    • PR
    • Merged
    • Tagged
  • Compose
    • PR
    • Merged
    • Tagged
  • PhyloNetworks
    • PR
    • Merged
    • Tagged
  • BasisMatrices
    • PR
    • Merged
    • Tagged

Edit: removed Text.jl as it's not registered and hasn't been updated in 10 months.
Edit: removed BioSequences.jl as I checked that they don't rely on ordering for product.

@ararslan

This comment has been minimized.

Member

ararslan commented Jun 24, 2018

@iamed2 Is this ready to merge?

@iamed2

This comment has been minimized.

Contributor

iamed2 commented Jun 25, 2018

I made some changes (simplifications) so I should push those first

@iamed2 iamed2 merged commit 3261b0e into master Jun 25, 2018

4 checks passed

codecov/patch 94.8% of diff hit (target 88.64%)
Details
codecov/project 95.8% (+7.16%) compared to a2b4ed3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ararslan ararslan deleted the ed/iterate branch Jun 25, 2018

@ararslan

This comment has been minimized.

Member

ararslan commented Jun 25, 2018

Thanks for your amazing work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment