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

Fix StatsModels #16

Closed
wants to merge 2 commits into from
Closed

Fix StatsModels #16

wants to merge 2 commits into from

Conversation

nignatiadis
Copy link

@nignatiadis nignatiadis commented May 4, 2019

Hi! This should fix #10, although it might be more elegant to proceed as @kleinschmidt recommended by defining the two lines of logic in StatsModels and including here only:

function concrete_term(t::Term, xs::AbstractVector{<:EventTime}, ::Nothing)
    IdentityTerm(t.sym)
end

@kleinschmidt: Does it matter if we dispatch on concrete_term rather than schema?

@ararslan ararslan requested a review from kleinschmidt May 4, 2019 22:13
Copy link
Member

@kleinschmidt kleinschmidt left a comment

Choose a reason for hiding this comment

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

I think this fixes the problem in the short term but I'd rather fix it upstream by not converting to Float64 as aggressively for continuous terms. I won't have time to look at that for a few weeks though so if you need a short-term fix this will be fine. Otherwise I'd suggest keeping statsmodels upper bounded until there's a fix there.

@kleinschmidt
Copy link
Member

As for dispatching on concrete_term vs. schema, I think you've made the right choice here (and that confusion is why I renamed what used to be methods of schema to concrete_term instead...)

@nignatiadis
Copy link
Author

Sounds great, thanks! The new formula system is really cool and what I had been missing most in Julia and ... one of these days (maybe in June) I will find the time to properly implement B/natural-splines..

@ararslan
Copy link
Member

Superseded by #17

@ararslan ararslan closed this 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