Replace term tuples with term vectors to reduce compilation latency - #354
Open
matthieugomez wants to merge 2 commits into
Open
Replace term tuples with term vectors to reduce compilation latency#354matthieugomez wants to merge 2 commits into
matthieugomez wants to merge 2 commits into
Conversation
- Represent term collections as Vector{AbstractTerm}: + returns a vector,
InteractionTerm and MatrixTerm store Vector{AbstractTerm}, and the rhs
built by ~ / @formula is always a vector (a lone term is wrapped)
- Apply schemas by looping over the vector, so methods compile once per
term type instead of once per formula shape
- Define content-based == and hash for FormulaTerm, InteractionTerm, and
MatrixTerm (vector fields break the egal fallback)
- Remove TupleTerm; TermOrTerms is now Union{AbstractTerm, AbstractVector{<:AbstractTerm}}
- Update tests and doctests; bump version to 0.8.0
- NEWS.md entry for 0.8.0 with the two breaking changes - hash for FunctionTerm consistent with == - Empty term vectors: width 0 MatrixTerm, n-by-0 model matrix; empty InteractionTerm is an error - Tests for every vector-of-terms method
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fitting a model with a never-seen formula currently costs ~0.1–0.8s of compilation, even when every term type is already precompiled. The reason is that formulas are encoded as tuples, so
apply_schema,modelcols,coefnames, etc. get a fresh specialization for every distinct number and order of terms, and likewise every method taking aMatrixTerm{Ts<:TupleTerm}.This PR represents term collections as
Vector{AbstractTerm}instead of tuples of terms (or a single term when there was only one variable in the formula). This is a breaking change (0.8.0).Changes
+on terms returnsVector{AbstractTerm}.~(and@formula) always makes the rhs a vector, even for a single term (@formula(y ~ x).rhs == [term(:x)]), removing the lone-term special case. The lhs stays a bare term, and afterapply_schemathe rhs still collapses to a bareMatrixTerm, soy, X = modelcols(f, data)is unchanged.InteractionTermandMatrixTermare no longer parametric and store aVector{AbstractTerm}.collect_matrix_termsreturns a vector in the mixed matrix/non-matrix case.apply_schemaover a vector loops term by term, with the same left-to-rightFullRanksemantics as the tuple broadcast.TupleTermis removed;TermOrTermsis nowUnion{AbstractTerm, AbstractVector{<:AbstractTerm}}.==andhashforFormulaTerm,InteractionTerm, andMatrixTerm, since vector fields break the default===fallback. This also fixes formula equality afterapply_schema(a former@test_broken).FunctionTermgets ahashconsistent with its existing==, sounique/Setdeduplicate formulas with function calls.MatrixTermhas width 0 and itsmodelcolsis an n×0 matrix (likeInterceptTerm{false});coefnamesof an empty vector isString[]; anInteractionTermwith no terms throws anArgumentError.The Documentation CI job fails at environment resolution, not at the doctests: GLM's compat is
StatsModels = "0.7", so the docs environment cannot pick up 0.8.0 until GLM bumps compat. The doctests pass locally.Latency
Timings below are for the first
modelmatrixcall on a formula shape not seen before, in a warm session on Julia 1.12, after warming up the pipeline withy ~ 1 + aandy ~ 1 + a + e:StatsModels 0.7.10: every new shape recompiles the pipeline (~99.9% compilation time):
This PR:
Downstream impact
I ran the test suites of the main reverse dependencies against this branch.
TupleTermis gone (fix is a one-line signature swap toAbstractVector{<:AbstractTerm}per method): RegressionFormulae, MixedModels, RCall, AnovaBase, HurdleDMR, MixedModelsSerialization.isa MatrixTerm{Tuple{...}}checks), each a few lines: Econometrics, FixedEffectModels, ShiftShares, Metida, SurvivalAnalysis.The only substantive migration is in MixedModels: the
InteractionTerm{<:NTuple{N,CategoricalTerm}}dispatch (randomeffectsterm.jl:127,210) has to become a runtime check (all(t -> t isa CategoricalTerm, it.terms)), since interaction element types no longer live in the type domain.