termnames #299

palday · 2023-09-02T02:28:39Z

xref JuliaStats/GLM.jl#428

src/statsmodel.jl

test/statsmodel.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

src/statsmodel.jl

test/statsmodel.jl

src/statsmodel.jl

src/contrasts.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

src/contrasts.jl

src/statsmodel.jl

test/statsmodel.jl

src/statsmodel.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

src/contrasts.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

kleinschmidt

I get why this functionality is needed vs. coefnames (one string per "variable"), and I think StatsModels is a good home for it, but I'm not sure termnames is quite right. some things that are terms never get named (tupleterms, matrixterms, formulaterm), some things that contain terms forward to their children while others do not (intercept term).

What about something like variablenames? I think that better captures what folks mean when they want this, without introducing confusion about what is and is not an AbstractTerm in the statsmodels system...

src/statsmodel.jl

kleinschmidt · 2023-09-05T16:19:18Z

src/statsmodel.jl

+termnames(::InterceptTerm{H}) where {H} = H ? "(Intercept)" : nothing
+termnames(t::ContinuousTerm) = string(t.sym)
+termnames(t::CategoricalTerm) = string(t.sym)
+termnames(t::Term) = string(t.sym)
+termnames(t::ConstantTerm) = string(t.n)
+termnames(t::FunctionTerm) = string(t.exorig)
+# termnames(TupleTerm)) always returns a vector, even if it's just one element, e.g.,
+# termnames((term(:a),))
+termnames(ts::TupleTerm) = mapreduce(termnames, vcat, ts; init=String[])
+# termnames(MatrixTerm)) always returns a vector, even if it's just one element, e.g.,
+# termnames(MatrixTerm(term(:a)))
+termnames(t::MatrixTerm) = mapreduce(termnames, vcat, t.terms; init=String[])
+termnames(t::InteractionTerm) =
+    only(kron_insideout((args...) -> join(args, " & "), vectorize.(termnames.(t.terms))...))


these all shoudl be in terms.jl I think, with other term API functions

kleinschmidt · 2023-09-05T16:21:34Z

src/statsmodel.jl

+Return value is either a `String`, an iterable of `String`s or nothing if there
+no associated name (e.g. `termnames(InterceptTerm{false}())`).


this is an interesting edge case. there is an associated name for the term, even if there's no coefficient that corresponds in the model.

src/statsmodel.jl

palday · 2023-09-05T16:41:12Z

@nalimilan I’m not opposed to the variablenames bit but I still think we should deprecate the internal termnames in favor of coefnames because it also didn’t refer to terms but rather coefs.

Thoughts on using variablenames instead of termnames? I'm pretty neutral and @kleinschmidt has a point.

palday · 2023-09-05T16:42:21Z

@kleinschmidt GH won't let me reply to the comment so....

I'm a bit worried by teh inconsistency with coefnames here, where an empty vector is used. Also, vcat with nothing will add an explicit nothing into the result. but perhaps that's intentional?

Yeah, I don't think that most users should call these methods directly, and String[] plays nicer with vcat, which is called if applying this to a collection of terms / formula. I'll update when we get Milan's thoughts on the other bit of bikeshed. 😄

Co-authored-by: Dave Kleinschmidt <[email protected]>

nalimilan · 2023-09-05T17:04:49Z

variablenames could mean something different: in R, all.vars(f) returns the names of variables which are used in a formula, which allows doing e.g. subset(df, all.vars(f)) to keep only useful variables. termnames sounds closer to what we want but I would have to double-check the formula machinery to have a definitive opinion. MatrixTerm and TupleTerm are kind of meta-terms wrapping other terms, right? So it makes sense that they don't have names themselves.

…nto pa/termnames

palday · 2023-09-05T17:23:39Z

I'm open to other names -- it's easy enough to change variablenames to anything else. The hard part was termnames to variablenames because that wasn't a universal replacement.

src/deprecated.jl

Co-authored-by: Alex Arslan <[email protected]>

palday · 2023-09-06T07:50:47Z

@kleinschmidt @nalimilan we could also go with the hybrid function name termvarnames. Or make this a completely internal method _variablenames because we're really only using it for gvif. We can always make it public later.

palday · 2023-09-06T21:06:28Z

okay, it seems like @kleinschmidt is now onboard with termnames. Great!

kleinschmidt

looks like docs are not picking up the termnames crossref: https://github.com/JuliaStats/StatsModels.jl/actions/runs/6102408363/job/16561033364?pr=299#step:5:279

fix that and then LGTM.

nalimilan · 2023-09-07T07:30:01Z

Looks like you haven't noticed my last comment at #299 (comment). Not a big deal, but maybe still worth adjusting?

termnames

a75b20b

palday requested review from kleinschmidt and nalimilan September 2, 2023 02:29

fix

dd1f431

nalimilan reviewed Sep 2, 2023

View reviewed changes

palday and others added 3 commits September 2, 2023 18:46

Apply suggestions from code review

6d727f4

Co-authored-by: Milan Bouchet-Valat <[email protected]>

more docs

3fef311

more docs more tests

922235f

palday requested a review from nalimilan September 2, 2023 19:26

nalimilan reviewed Sep 3, 2023

View reviewed changes

src/statsmodel.jl Outdated Show resolved Hide resolved

src/statsmodel.jl Outdated Show resolved Hide resolved

test/statsmodel.jl Outdated Show resolved Hide resolved

src/statsmodel.jl Outdated Show resolved Hide resolved

src/contrasts.jl Outdated Show resolved Hide resolved

palday and others added 6 commits September 3, 2023 20:32

Update src/statsmodel.jl

f517329

Co-authored-by: Milan Bouchet-Valat <[email protected]>

use StatsAPI directly, deprecate old termnames

cf0aa59

consistency

1b8d89d

argh

987fce6

StatsAPI

767d6e0

init with String[]

44dc3c3

nalimilan reviewed Sep 5, 2023

View reviewed changes

palday and others added 3 commits September 5, 2023 10:28

okidoki

e71ba22

Co-authored-by: Milan Bouchet-Valat <[email protected]>

deprecations

fe426f0

whitespace

4ce8532

nalimilan approved these changes Sep 5, 2023

View reviewed changes

src/contrasts.jl Outdated Show resolved Hide resolved

Update src/contrasts.jl

750dcd2

Co-authored-by: Milan Bouchet-Valat <[email protected]>

kleinschmidt reviewed Sep 5, 2023

View reviewed changes

Update src/statsmodel.jl

5c28fcf

Co-authored-by: Dave Kleinschmidt <[email protected]>

palday added 3 commits September 5, 2023 19:12

variablenames and xref

73c312d

Merge branch 'pa/termnames' of github.com:JuliaStats/StatsModels.jl i…

960abae

…nto pa/termnames

fixes

e482906

palday added 2 commits September 5, 2023 19:20

move to a different file

561d377

update doc string

e744aa2

ararslan reviewed Sep 5, 2023

View reviewed changes

src/deprecated.jl Outdated Show resolved Hide resolved

sure

28e3b6a

Co-authored-by: Alex Arslan <[email protected]>

palday added 2 commits September 6, 2023 22:55

s/variablenames/termnames/

80511d4

@ararslan making trouble

7f41293

kleinschmidt self-requested a review September 6, 2023 21:20

kleinschmidt approved these changes Sep 6, 2023

View reviewed changes

include termnames in manual

0dccffb

palday merged commit 1632bff into master Sep 6, 2023
6 of 7 checks passed

palday deleted the pa/termnames branch September 6, 2023 21:34

nalimilan mentioned this pull request Sep 7, 2023

Slighltly improve coefnames with no intercept #301

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

termnames #299

termnames #299

palday commented Sep 2, 2023

kleinschmidt left a comment

kleinschmidt Sep 5, 2023

palday Sep 5, 2023

kleinschmidt Sep 5, 2023

palday commented Sep 5, 2023

palday commented Sep 5, 2023

nalimilan commented Sep 5, 2023 •

edited by ararslan

Loading

palday commented Sep 5, 2023

palday commented Sep 6, 2023

palday commented Sep 6, 2023

kleinschmidt left a comment

nalimilan commented Sep 7, 2023

		Return value is either a `String`, an iterable of `String`s or nothing if there
		no associated name (e.g. `termnames(InterceptTerm{false}())`).

termnames #299

termnames #299

Conversation

palday commented Sep 2, 2023

kleinschmidt left a comment

Choose a reason for hiding this comment

kleinschmidt Sep 5, 2023

Choose a reason for hiding this comment

palday Sep 5, 2023

Choose a reason for hiding this comment

kleinschmidt Sep 5, 2023

Choose a reason for hiding this comment

palday commented Sep 5, 2023

palday commented Sep 5, 2023

nalimilan commented Sep 5, 2023 • edited by ararslan Loading

palday commented Sep 5, 2023

palday commented Sep 6, 2023

palday commented Sep 6, 2023

kleinschmidt left a comment

Choose a reason for hiding this comment

nalimilan commented Sep 7, 2023

nalimilan commented Sep 5, 2023 •

edited by ararslan

Loading