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

Move most combinatorics code out of Base #13897

Merged
merged 1 commit into from
Nov 9, 2015
Merged

Move most combinatorics code out of Base #13897

merged 1 commit into from
Nov 9, 2015

Conversation

jiahao
Copy link
Member

@jiahao jiahao commented Nov 6, 2015

This PR extracts all combinatorics functions from Julia's Base library, except for:

  • binomial,
  • factorial, and
  • gamma
  • edit: and basic functions for permutations: isperm, invperm, permute!, ipermute!
  • and nextprod which is used in Base.DSP

All other functions are to be migrated to https://github.com/JuliaLang/Combinatorics.jl

See: JuliaMath/Combinatorics.jl#8

Ref: #5155

jiahao added a commit to JuliaMath/Combinatorics.jl that referenced this pull request Nov 6, 2015
- DROP 0.4 SUPPORT
- Import most of base/combinatorics.jl (Ref: JuliaLang/julia#13897)
- Move most of the special numbers to numbers.jl
- Put combinations, permutations and partitions in their own files
- Rename special numbers with ~num suffix. This renaming is particularly
  important for catalannum to avoid clashing with the Base.catalan
  irrational constant.
@tkelman
Copy link
Contributor

tkelman commented Nov 6, 2015

these should throw specific errors saying they're now in Combinatorics.jl

the permutation functions like invperm that work on vectors of ints might be small and generally useful enough to leave in for now, but the special types should move.

@jiahao
Copy link
Member Author

jiahao commented Nov 6, 2015

How should we handle the deprecations? The easiest thing I can think of is to add catchall methods like combinations(args...) = error("You need Combinatorics.jl now.") to deprecated.jl.

@JeffBezanson
Copy link
Member

Agree; I would keep isperm, invperm, permute!, and ipermute!. I could go either way on factorial and gamma. Unfortunately the DSP code uses nextprod. Not sure what to do about that.

@jakebolewski
Copy link
Member

Put the DSP code in a package?

@tkelman
Copy link
Contributor

tkelman commented Nov 6, 2015

I like the phrasing that was done for the PkgDev moves

register(args...) =

Though since these were exported, want to make sure Combinatorics.jl can extend them properly.

@jiahao
Copy link
Member Author

jiahao commented Nov 6, 2015

Looks like rudimentary support for permutations is needed in Base, so I'll plan to put back

  • isperm
  • invperm
  • permute!/!!
  • ipermute!/!!

I'll also leave nextprod in with a note about its removal when DSP is removed.

All other methods are moved to Combinatorics.jl
@jiahao jiahao changed the title WIP: migrate all combinatorics code out of Base Move most combinatorics code out of Base Nov 6, 2015
@jiahao
Copy link
Member Author

jiahao commented Nov 6, 2015

Should be ready for review

jakebolewski added a commit that referenced this pull request Nov 9, 2015
Move most combinatorics code out of Base
@jakebolewski jakebolewski merged commit 86cb6e8 into master Nov 9, 2015
@jakebolewski jakebolewski deleted the cjh/rm-combi branch November 9, 2015 21:21
@KristofferC
Copy link
Member

Should there be a news entry for this?

@tkelman
Copy link
Contributor

tkelman commented Nov 9, 2015

Yes.

jakebolewski added a commit that referenced this pull request Nov 9, 2015
@@ -1044,13 +1044,6 @@ Compute the minimum absolute values over the singleton dimensions of `r`, and wr
minabs!

doc"""
prevprod([k_1,k_2,...], n)
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be manually removed from the rst docs. Again, keeping them in sync at the same time is a much clearer way to go right now.

@sbromberger
Copy link
Contributor

This introduces a problem: in 0.4 0.5, when using Combinatorics is included (for combinations()), there's an error since there's now a name conflict with Base. I guess this is unavoidable?

@yuyichao
Copy link
Contributor

No it doesn't introduce any problem on 0.4. Combinators 0.3.0 requires julia 0.5+ and you shouldn't use it with julia 0.4.

@sbromberger
Copy link
Contributor

I misspoke - it's actually nightly builds that are affected:

WARNING: both Combinatorics and Base export "combinations"; uses of it in module LightGraphs must be qualified
...
ERROR: LoadError: LoadError: UndefVarError: combinations not defined

See https://travis-ci.org/JuliaGraphs/LightGraphs.jl/jobs/90627799 for details.

@jiahao
Copy link
Member Author

jiahao commented Nov 12, 2015

Yes, you will have to bear with the warning until the nightly builds are updated. A temporary workaround is to qualify the usage as the warning suggests:

import Combinatorics: combinations

I do this in the Combinatorics tests on master.

@tkelman
Copy link
Contributor

tkelman commented Nov 12, 2015

The nightly in that run was from today. I think Combinatorics.jl should be checking if isdefined(Base, :combinations) and if so importing it to extend it with new methods.

@sbromberger
Copy link
Contributor

Warnings I can handle, but it's followed by an error (see edited comment). It stinks because the "failed build" badge shows up on the project's home page as a result, and integration tests for PRs are all failing. I'm fully qualifying it now.

@jiahao
Copy link
Member Author

jiahao commented Nov 12, 2015

Oh good point. I'll have to import all these functions from Base for the entire 0.5 cycle since there will still be the methods left that throw the errors.

@tkelman
Copy link
Contributor

tkelman commented Nov 12, 2015

Yes, though it'll be a little more future-proof if you use isdefined or a try-catch rather than version number comparisons.

@sbromberger
Copy link
Contributor

Thanks for the quick response. Let me know if/how I can help.

@jiahao
Copy link
Member Author

jiahao commented Nov 12, 2015

I'm not going to write isdefined(:...) for 8 different functions.

@tkelman
Copy link
Contributor

tkelman commented Nov 12, 2015

Please don't use a version number comparison, do a feature check. Otherwise the versions of the package you write now are guaranteed to break as soon as the deprecations get removed.

edit: hrm, maybe I spoke too soon, importing something nonexistent is only a warning? Did not expect that. Still, version number checks are just generally brittle and bad practice, and don't work when you do a NO_GIT build.

jiahao added a commit to JuliaMath/Combinatorics.jl that referenced this pull request Nov 12, 2015
On 0.5, the 8 functions excised from Base are still defined, as they
have methods that print the error to use Combinatorics.jl.

Ref:
JuliaLang/julia#13897 (comment)
jiahao added a commit to JuliaMath/Combinatorics.jl that referenced this pull request Nov 12, 2015
On 0.5, the 8 functions excised from Base are still defined, as they
have methods that print the error to use Combinatorics.jl.

Ref:
JuliaLang/julia#13897 (comment)
@jiahao
Copy link
Member Author

jiahao commented Nov 12, 2015

I've included an isdefined check on Base.combinations, on the premise that we will excise all 8 deprecation errors at once.

jiahao added a commit to JuliaMath/Combinatorics.jl that referenced this pull request Nov 12, 2015
On 0.5, the 8 functions excised from Base are still defined, as they
have methods that print the error to use Combinatorics.jl.

Ref:
JuliaLang/julia#13897 (comment)
@sbromberger
Copy link
Contributor

OK, I'm a bit confused here. How is one supposed to write code that uses combinations() that works on 0.4 and 0.5? I can't put Combinatorics.jl in REQUIRE, since trying to Pkg.add() it causes a 0.4 failure ("ERROR: Combinatorics can't be installed because it has no versions that support 0.4.2-pre+3 of julia."), and omitting it in 0.5 throws the error about having combinations() now in Combinatorics.

Advice greatly appreciated. Thanks.

@jpfairbanks
Copy link
Contributor

Could you tag a version of Combinatorics.jl that only supports 0.4.X and just re-exports the functions from Base?

@yuyichao
Copy link
Contributor

There shouldn't be any problems. Combinatorics 0.2.0 should work fine on 0.4.0.

Tested on JuliaBox

julia> Pkg.update()                                                              
INFO: Initializing package repository /home/juser/.julia/v0.4
INFO: Cloning METADATA from git://github.com/JuliaLang/METADATA.jl
INFO: Updating METADATA...
INFO: Computing changes...
INFO: No packages to install, update or remove

julia> Pkg.add("Combinatorics")                                                  
INFO: Cloning cache of Combinatorics from git://github.com/JuliaLang/Combinatoric
s.jl.git
INFO: Updating cache of Compat...
INFO: Cloning cache of Iterators from git://github.com/JuliaLang/Iterators.jl.git
INFO: Cloning cache of Polynomials from git://github.com/Keno/Polynomials.jl.git
INFO: Installing Combinatorics v0.2.0
INFO: Installing Compat v0.7.7
INFO: Installing Iterators v0.1.9
INFO: Installing Polynomials v0.0.4
INFO: Package database updated

Note that you shouldn't require Combinatorics 0.3.0 or higher.

@sbromberger
Copy link
Contributor

Ah, that's likely the issue - I had 0.3.1 specified in REQUIRE. Let's see what happens when I remove the version.

@tkelman
Copy link
Contributor

tkelman commented Nov 13, 2015

The issue is that Combinatorics 0.2.0 only supports releases of 0.4 JuliaLang/METADATA.jl@b7b955a, and Combinatorics 0.1.2 and earlier require Polynomial which is deprecated on 0.4. @sbromberger can you safely upgrade from an rc to release 0.4.1?

@sbromberger
Copy link
Contributor

@tkelman I can, but what about the users of the package? I'd hate to leave folks out in the cold.

ETA: and I'm on 0.4.2-pre+3, so I'm a bit puzzled.

@yuyichao
Copy link
Contributor

@tkelman It's 0.4.2-pre not 0.4.0-rc2

@tkelman
Copy link
Contributor

tkelman commented Nov 13, 2015

Actually Polynomial should only be deprecated for releases of 0.4, so my mistake, the issue was the version number of Combinatorics you were trying to require. Pkg resolution failures don't always give the most informative errors right now, there are one or two issues on this.

@sbromberger
Copy link
Contributor

Thanks for the prompt responses (as usual!) - testing LG now without version numbers in Combinatorics.

There are lots of moving parts these days, and trying to keep the packages testing cleanly on both 0.4 and 0.5 is taxing work.

ETA: ok, removing the version number fixes the problems on 0.4 and 0.5. (now to figure out the other failures.) Thanks again.

@coveralls
Copy link

Coverage Status

Coverage decreased (-20.8%) to 61.053% when pulling 93911dd on cjh/rm-combi into 94f31d1 on master.

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.

9 participants