Skip to content

ordinal regression model type & polr engine #8

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

corybrunson
Copy link
Owner

This PR continues #6; see there for details.

Copy link
Collaborator

@topepo topepo left a comment

Choose a reason for hiding this comment

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

Overall it looks good. @ me if you have questions or need help with anything.

A few random things unrelated to the PR:

  • The dev version of yardstick has the function ranked_prob_score that is specific to ordinal models.
  • Once this package is on CRAN, I’ll make “engine” documentation files for each model/engine combo here. Here’s an example. I’ll add you to review so you can check them and make edits as you see fit.
  • Before sending to CRAN, check out Davis's extrachecks repo.

functionality. See below for examples of fitting ordinal regression models
with {censored}.
}
\examples{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added some notes about "noSuggests" to the test files. That comes up here too.

There are data and packages that are used here and CRAN will complain since the packages may not be installed by the user.

There are two ways to deal with this.

  1. Wrap the whole thing in a loop that uses if (rlang::is_installed(c("MASS", "ordinalForest"))). That's easy and here are examples
  2. More crafty is to prevent CRAN from seeing the examples. This might bite you on the initial submission but here is how to do that.

Version: 0.0.0.9000
Authors@R: c(
person("Max", "Kuhn", , "[email protected]", role = c("aut", "cre"),
comment = c(ORCID = "0000-0003-2402-136X")),
person("Jason Cory", "Brunson", , "[email protected]", role = c("aut"),
comment = c(ORCID = "0000-0003-3126-9494")),
person("Posit Software PBC", role = "cph")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this since it is yours.

I'd also run usethis::use_tidy_description() to reorganize this file.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@topepo i'm not sure what you mean by "remove this".

Also, should i mark myself "cre" for the time being or wait until someone is ready to submit to CRAN?

#' functionality. See below for examples of fitting ordinal regression models
#' with {censored}.
#'
#' @examples
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same things related to "examplesif" and is_installed() apply here.

#' {ordered} provides engines for ordinal regression models for the {parsnip}
#' package. The models may have cumulative, sequential, or adjacent-category
#' structure, and in future these may be disaggregated into separate model
#' types. A vignette will provide thorough illustrations of {ordered}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a vignette 😄

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yet! Should we require a vignette prior to initial submission?

type = "class",
value = list(
pre = NULL,
post = NULL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to reconvert the predictions to be ordered factors?

@corybrunson
Copy link
Owner Author

@topepo from #6:

My first thought is that the bare skeleton of ordinal_reg() should live in parsnip so that they can use our "enhanced" engine documentation.

This branch might not be in final form, but should i synchronize the fork and submit a PR (and into what branch of parsnip)?

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.

3 participants