Closed lxvm closed 7 months ago
Thanks! Yes, I think it's a good idea. We should try to keep it working for Julia v1.6 as well as long as it's the LTS. In order to do that, we can simply keep the old behavior on Julia v1.6 as done in NLopt: https://github.com/JuliaOpt/NLopt.jl/blob/master/src/NLopt.jl#L633-L637 So the behavior in Julia v1.6 remains the same: the load time is not decreased even when JuMP is not used but in Julia v1.9 it's using the extension only if JuMP is loaded.
Can you post timings for comparison with Julia v1.9 or v1.10 ?
I think GenericLinearAlgebra is probably not a good candidate for this but RecipesBase seems to be a good one. GeometryBasics
as well.
Attention: 7 lines
in your changes are missing coverage. Please review.
Comparison is base (
268c701
) 88.95% compared to head (9493f37
) 88.92%.
Files | Patch % | Lines |
---|---|---|
ext/PolyhedraJuMPExt.jl | 75.00% | 6 Missing :warning: |
src/Polyhedra.jl | 0.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@blegat Thank you for your recommendations regarding compatibility and additional extensions. I have used the strategy in NLopt to maintain 1.6 compatibility and I also moved RecipesBase and GeometryBasics to extensions.
Although these changes could be considered breaking in Julia 1.9 and above, the recommended usage in the docs and examples always loads the extension packages and so I expect these changes wouldn't impact very many users. One exception is that 3d plotting would require loading the plotting package (Makie/MeshCat) before calling Polyhedra.Mesh
. If you think these problems are serious then this pr could go into a breaking, minor release. (Notably, I changed Polyhedra.Mesh
from a struct to a function that returns a struct defined in the GeometryBasics extension.)
For example, the following works:
using Polyhedra, GLMakie
v = convexhull([0, 0, 0]) + conichull([1, 0, 0], [0, 1, 0], [0, 0, 1])
p = polyhedron(v)
m = Polyhedra.Mesh(p)
mesh(m; color=:blue)
but this would no longer work
using Polyhedra
v = convexhull([0, 0, 0]) + conichull([1, 0, 0], [0, 1, 0], [0, 0, 1])
p = polyhedron(v)
m = Polyhedra.Mesh(p)
using GLMakie
mesh(m; color=:blue)
Starting from this gist I wrote some tests for precompilation times with the help of the juliaup installer
Relevant results are summarized here, taken from the raw output below
Branch | Julia version | Pkg.add (s) |
using Polyhedra (s) |
---|---|---|---|
ext | 1.6.7 | 97.245989 | 4.532855 |
ext | 1.10.0 | 74.781329 | 1.366950 |
master | 1.6.7 | 101.986754 | 5.395770 |
master | 1.10.0 | 88.853759 | 1.687544 |
It looks like the speed-up of updating from v1.6.7 to v1.10.0 is more relevant than the improvements from moving some dependencies to extensions, although I notice Pkg.add
is 14 seconds faster with this pr on v1.10.0, which is what I was hoping for.
Also, regarding the CI, I think it breaks on v1.10 due to changed printing of rationals and the documentation build broke because I originally set the version to 0.8 and have since reverted to 0.7.7, assuming this can be considered non-breaking.
I think the breaking change with Mesh
is fine, we can release a new breaking release in Polyhedra easily since we haven't hit v1.0 yet and it makes sense. I would add an error in case someone write Mesh(p)
before GeometryBasics is loaded saying that it needs to be loaded first before using Mesh
. Otherwise, someone might loose a lot of time debugging wondering why suddenly Mesh
isn't available while on the other script he has it was ^^ I even think this error should remain there forever and not just be there because of the breaking change.
Can you rebase on top of the master branch ?
@blegat I've rebased, uncommented the lines you requested, and added an error message for Mesh
so that
using Polyhedra
v = convexhull([0, 0, 0]) + conichull([1, 0, 0], [0, 1, 0], [0, 0, 1])
p = polyhedron(v)
m = Polyhedra.Mesh(p)
throws
ERROR: this method requires using GeometryBasics
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] Mesh(p::DefaultPolyhedron{Rational{…}, Polyhedra.Intersection{…}, Polyhedra.Hull{…}})
@ Polyhedra ~/.julia/dev/Polyhedra/src/Polyhedra.jl:115
[3] top-level scope
@ REPL[4]:1
Some type information was truncated. Use `show(err)` to see complete types.
I took the opportunity of making a breaking change to run the Aqua tests, which most notably caught the type piracy in the FullDim
union type / function. I renamed the union type to FullDims
and am happy to change it if necessary. I know for a fact that this will cause CDDLib.jl to break, although the test suite still passes on my machine. I could follow up with prs to the Polyhedra.jl dependencies.
Moreover, this pr's changes are now summarized in a NEWS.md file
Indeed, it's type piracy. You can move this to a separate PR if you don't want it to block this one. We could make a breaking change and then a breaking release so that CDDLib is allowed to be updated. However, I would rather keep FullDim
for the type and rename the function. The function had the same since it was like a constructor. If it's not a constructor, its naming should be lower case with underscores, e.g., typed_fulldim
. The only difference between typed_fulldim
and fulldim
is that typed_fulldim
gives a StaticArrays.Size
if the arrays are StaticArrays
Sounds good, I think it would be better to deal with the issues Aqua raises in a separate pr to get this one through, so I'll revert those commits to keep only the changes to extensions.
We should be clear to run the tests again. If all goes well, I think we should revisit the version number and news file.
This pr reduces Polyhedra.jl dependencies by moving the functionality it uses with JuMP.jl to an extension package.
These changes will be breaking because it now requires loading JuMP and requires Julia v1.9.See the checklist below for what needs to be done:or GenericLinearAlgebra.jland GeometryBasics.jl)detect_new_linearities
?I would appreciate additional feedback from maintainers on the utility of this pr as well as its approach. Thank you!