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

Depend on IntervalSets.jl rather than Intervals.jl? #374

Closed
johnbcoughlin opened this issue Dec 29, 2021 · 6 comments
Closed

Depend on IntervalSets.jl rather than Intervals.jl? #374

johnbcoughlin opened this issue Dec 29, 2021 · 6 comments

Comments

@johnbcoughlin
Copy link

There are two packages in the ecosystem for representing operations on non-iterable intervals, IntervalsSets.jl and Intervals.jl.

The latter currently depends on TimeZones.jl directly, which is a heavy transitive dependency for pure-math projects which want to use Polynomials. (I've opened an issue to address this). It is not as light-weight and includes functionality for plotting and printing intervals, as well as anchored intervals. Finally, it is not as widely used in the math ecosystem. See below for the list of each package's dependees.

For all these reasons, it would be nice to switch to using IntervalSets. Unfortunately, this will be a breaking change. Polynomials.jl depends on Intervals via the domain function.

What are the thoughts of the core maintainers on this, and would it be something that you'd consider addressing in a future breaking release?

julia> direct_dependents("Intervals")
10-element Vector{String}:
 "ConstraintDomains"
 "DateSelectors"
 "EconoSim"
 "Empirikos"
 "IndependentHypothesisWeighting"
 "LibPQ"
 "PatternFolds"
 "Polynomials"
 "SoapySDR"
 "SpecialPolynomials"

julia> direct_dependents("IntervalSets")
53-element Vector{String}:
 "AbstractPlotting"
 "ApproxFun"
 "ApproxFunBase"
 "ApproxFunFourier"
 "ApproxFunOrthogonalPolynomials"
 "ApproxFunSingularities"
 "AxisArrays"
 "AxisIndices"
 "AxisKeys"
 "BAT"
 "BasicBSpline"
 "CalculusWithJulia"
 "CamiXon"
 "ClassicalOrthogonalPolynomials"
 "ClimaCore"
 "CompactBases"
 "ComplexPhasePortrait"
 "ContinuumArrays"
 "CoulombIntegrals"
 "DirectionalStatistics"
 "DomainIntegrals"
 "DomainSets"
 "DungAnalyse"
 "DungBase"
 ⋮
 "InventoryManagement"
 "Khepri"
 "Makie"
 "MartaCT"
 "MeasureTheory"
 "OrthogonalPolynomialsQuasi"
 "PICDataStructures"
 "Poltergeist"
 "ProfileView"
 "RadiationDetectorSignals"
 "RadiationSpectra"
 "RayTracing"
 "ReinforcementLearningEnvironments"
 "ReinforcementLearningExperiments"
 "ReinforcementLearningZoo"
 "SampledSignals"
 "SolidStateDetectors"
 "StaticRanges"
 "StatsDiscretizations"
 "StatsMakie"
 "TimeAxes"
 "ValueShapes"
 "YAXArrays"
@jverzani
Copy link
Member

Thanks for this. I’ll have a look. I don’t recall it being a critical dependency, so would expect only the inconvenience of a breaking release would be the cost of changing.

@johnbcoughlin
Copy link
Author

Thanks! Let me know if you're busy and would like me to work on it for this and the other repo.

@jverzani
Copy link
Member

If you wanted to help that would be great. My first thought would be to just keep domain as a non-exported method and not extend the one in Intervals.jl. My guess is that would be just a matter of removing the dependency, as I don't expect that we take advantage of any default behaviour.

Although I really wish there was just one choice for interval support...

@johnbcoughlin
Copy link
Author

I'm a little confused--domain is already exported, see the export statements in common.jl. Are you saying that all that Polynomials.jl needs to do is change the type of the Interval constructed by methods like domain(::Type{<:ChebyshevT})? That might be as simple as switching using Intervals for using IntervalSets, but would constitute a breaking change for package dependees who expect an Intervals.Interval.

In the issues of both IntervalSets and Intervals, there are discussions about combining the two packages, but it seemed to me there was some disagreement about which should be the "basic" package...

@jverzani
Copy link
Member

I'm working on a PR that removes the dependency and implements a simple version of Intervals with the basic interface of first, last, in,isopen, and isbounded. Polynomials doesn't really use more than that, the use of Intervals to provide it was a convenience, but as you point out a heavy one. In the process I would not export domain so as not to conflict if Intervals is included, which is breaking. (Though I doubt too much code beyond SpecialPolynomials will be affected). I hope to have a PR by the afternoon. I'd appreciate any comments you may have.

@jverzani
Copy link
Member

I put in a PR here #377 to a new branch currently titled v3. I'd likely move the expected Julia version to 1.6 or later if making a breaking change.

jverzani added a commit that referenced this issue Dec 30, 2021
* work around domain

* remove dependency on Intervals

* version bump
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

No branches or pull requests

2 participants