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

Recursive Fix #187 & #174 #194

Merged
merged 5 commits into from
Aug 10, 2022
Merged

Conversation

AlbertoAlmuinha
Copy link
Contributor

Hi @mdancho84,

I have created a PR to resolve some of the open issues related to recursive models. Basically I have applied a condition that when in the test dataset there are more levels in the id than in the stored train_tail, then this object is filtered with the same levels available in the test dataset.

Anything we discuss.

Regards,

@AlbertoAlmuinha
Copy link
Contributor Author

Hi @mdancho84 ,

I have added another commit to solve issue #188 . Basically what I have done is instead of detecting specifically the class "_elnet", detect any class that contains the word "net" to send it by recursive or panel recursive. It is very related with the issue #76 solved some time ago.

Perhaps the way to dispatch can be made even more sophisticated, but I think that for the moment it may be a good approximation.

Regards,

@mdancho84
Copy link
Contributor

Awesome!! I appreciate the help. Let me take a look over the next day or so and I'll provide any comments.

@AlbertoAlmuinha
Copy link
Contributor Author

I just added a new commit to give a solution to the request #160 creating the function drop_modeltime_model.

Regards,

Copy link
Contributor

@mdancho84 mdancho84 left a comment

Choose a reason for hiding this comment

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

I reviewed and am OK. Things we might need to plan for:

  • The detect_net() I like this. But one thing I'm thinking is that if a model is not a glmnet style but has a class of "something_net", then it will get selected and might go through the glmnet prediction.

@mdancho84 mdancho84 merged commit ff32b26 into business-science:master Aug 10, 2022
@AlbertoAlmuinha
Copy link
Contributor Author

I thought the same yesterday, maybe it would be a better idea to look for the parsnip model classes with glmnet engines and encapsulate them under that function so there can be no confusion…

I'll take a look at it later

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.

2 participants