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

statsmodels compatibility #17

Merged
merged 4 commits into from
Jun 30, 2019
Merged

statsmodels compatibility #17

merged 4 commits into from
Jun 30, 2019

Conversation

kleinschmidt
Copy link
Member

@kleinschmidt kleinschmidt commented May 21, 2019

treat vectors of EventTimes like continuous terms. needs JuliaStats/StatsModels.jl#106 (to just copy continuous terms instead of converting to Float64).

fixes #10 , #16

treat vectors of EventTimes like continuous terms.  needs
JuliaStats/StatsModels.jl#106 (to just copy continuous terms instead of
converting to Float64).
@ararslan
Copy link
Member

You'll probably want to turn the StatsModels upper bound in the Project.toml into a lower bound in order for this to work, since concrete_term isn't defined without Son of Terms.

@ararslan
Copy link
Member

I guess we'll need to wait until StatsModels is tagged, since otherwise the StatsModels version requirement is unsatisfiable.

## StatsModels compatibility

StatsModels.concrete_term(t::Term, xs::AbstractVector{<:EventTime}, ::Nothing) =
StatsModels.ContinuousTerm(t.sym, first(xs), first(xs), first(xs), first(xs))
Copy link
Member

Choose a reason for hiding this comment

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

Why not actually compute the mean, variance, min and max?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that even defined for event times? How would the truncation be handled?

Maybe a better option is to add methods like mean(<:AbstractVector{<:EventTime})?

Copy link
Member

Choose a reason for hiding this comment

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

Hard to tell... But it also sounds weird to pass identical values. Maybe nothing should be allowed?

@kleinschmidt
Copy link
Member Author

Tests pass locally for me with StatsModels 0.6.0 but I've restarted the travis job to make sure...

@ararslan ararslan closed this Jun 27, 2019
@ararslan ararslan reopened this Jun 27, 2019
@ararslan
Copy link
Member

ararslan commented Jun 27, 2019

Has the StatsModels 0.6.0 tag not been merged to General or something?

@kleinschmidt
Copy link
Member Author

No it has but it lists julia >= 1 in the project.toml so that 0.7 build failed...do you care about 0.7 compatibility still?

@kleinschmidt
Copy link
Member Author

(I'll update the travis config and Project.toml in this PR if you want to drop 0.7 support...)

@ararslan
Copy link
Member

Down with 0.7! Down with 0.7!

@ararslan
Copy link
Member

Oh, I see, I was fooled because I was looking at the push build rather than the PR build. That's why I didn't see that it was only not working on 0.7.

@kleinschmidt
Copy link
Member Author

I guess also the version should be bumped right?

@ararslan
Copy link
Member

Yep

@ararslan ararslan merged commit f5ab838 into master Jun 30, 2019
@ararslan ararslan deleted the dfk/statsmodels6 branch June 30, 2019 19:52
@ararslan ararslan mentioned this pull request Jun 30, 2019
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.

Cox regression broke with latest StatsModels master
3 participants