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

add infer verbs? #10

Closed
topepo opened this issue Sep 10, 2018 · 8 comments
Closed

add infer verbs? #10

topepo opened this issue Sep 10, 2018 · 8 comments

Comments

@topepo
Copy link
Member

topepo commented Sep 10, 2018

generate, calculate, hypothesize, specify, and visualize are all pretty generic.

I think that these are all simple functions that take tibbles so it might require overloading the classes to get a specific method what infer wants to do.

@andrewpbray and @ismayc... any thoughts on this? This would enable others to use those verbs for their specific needs.

@ismayc
Copy link

ismayc commented Sep 10, 2018

I think this makes a lot of sense. I’m not exactly sure how others would use the verbs individually instead of in the {infer} pipeline though too, but I’m likely overlooking some use cases.

@hadley
Copy link
Member

hadley commented Sep 10, 2018

I don't think new functions should live here; this is specifically for base functions (or functions used by many existing packages) that should be generic. I think these functions would be a better fit for modelgenerics.

@ismayc
Copy link

ismayc commented Sep 10, 2018

I was assuming this repo and modelgenerics were the same thing based on the reroute. I'm not familiar with modelgenerics but agree with Hadley that these verbs aren't particularly useful unless some kind of modeling or testing is being done.

@topepo
Copy link
Member Author

topepo commented Oct 9, 2018

@ismayc we're probably going to send this to CRAN this week. Do you have any issues if I add those verbs? I don't see much downside.

@ismayc
Copy link

ismayc commented Oct 9, 2018

Nope. That works for us.

@topepo
Copy link
Member Author

topepo commented Oct 10, 2018

@ismayc Can you take a look at the new branch?

I kept the signatures the same (mostly x). Visualize uses data. If (or when) you make these methods in infer, would you consider changing that to x too?

@ismayc
Copy link

ismayc commented Oct 10, 2018

@topepo Looks good to me. We should probably switch to x. I’ll add that to the to-do list.

@topepo
Copy link
Member Author

topepo commented Oct 10, 2018

I'll change and do a PR. Thanks!

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

No branches or pull requests

3 participants