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

refactor model specification printing #739

Merged
merged 10 commits into from Jun 6, 2022
Merged

refactor model specification printing #739

merged 10 commits into from Jun 6, 2022

Conversation

simonpcouch
Copy link
Contributor

While working on some previous PRs, I've noticed that all but one or two of our model spec print methods take the form:

print.bag_mars <- function(x, ...) {
  cat("Bagged MARS Model Specification (", x$mode, ")\n\n", sep = "")
  model_printer(x, ...)

  if (is_printable_spec(x)) {
    cat("Model fit template:\n")
    print(show_call(x))
  }
  invisible(x)
}

where the only line that changes method-to-method is the cat header summarizing the model type.

This PR refactors model specification print methods to all dispatch to print.model_spec, which now makes use of a print_model_spec helper that subsets a tibble giving the descriptions for each class to change that model type description in the header. Hopefully we find this to be more maintainable.

This change should be non-breaking for dependencies.

  • If they define engines for a model type that is already supported in parsnip and rely on parsnip's print methods, we've tested that the output doesn't change. (Exceptions noted in 135a544"Model " is added preceding "Specification" in the header in 4 print methods.)
  • If they supply their own print methods, print.model_spec won't be dispatched to.

This PR still exports the new print_model_spec helper, though, in case dependencies want to make use of it. That code would look like:

print_model_spec(new_type_object, desc = "New Type Description")

This PR will be hard to review as a whole (lots of deletions across many files), so I tried to commit in such a way to make reviewing commit-by-commit much easier. :)

@EmilHvitfeldt
Copy link
Member

This is related to #567

@simonpcouch
Copy link
Contributor Author

Oh, sweet! Closes #567. You noted that same edge case, too. :)

@simonpcouch simonpcouch requested a review from hfrick May 31, 2022 19:41
Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

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

Nice one! Thanks for the commit order, that made it a lot easier indeed 🙌

Emil's comment #567 (comment) about the print method for the null model is still open. If we want this PR to close #567, we should address this or turn it into its own issue so that it doesn't get lost.

@simonpcouch
Copy link
Contributor Author

Ah, thanks for pointing that out.

It looks like the output of null_model doesn't actually dispatch to the print method mentioned in #567. All of the methods for null models are defined for class nullmodel, but the output of null_model has first class null_model. With the current PR (that doesn't adjust null model internals):

library(parsnip)

null_mod <- null_model()

class(null_mod)
#> [1] "null_model" "model_spec"

methods(class = "null_model")
#> no methods found
methods(class = "nullmodel")
#> [1] predict print   tidy   
#> see '?methods' for accessing help and source code

# dispatches to new method :(
null_mod
#> null model Model Specification (classification)

Created on 2022-06-01 by the reprex package (v2.0.1)

I could print.null_model <- print.nullmodel here, but I think a fresh issue re: null model dispatch is probably more appropriate.

@hfrick
Copy link
Member

hfrick commented Jun 1, 2022

I think nullmodel is the class for a fitted null model, while null_model is the class for the model spec. Does adding a row to model_descs for the null model solve the unfortunate printing for null_model()?

@simonpcouch
Copy link
Contributor Author

It does🐨

library(parsnip)

null_model()
#> Null Model Specification (classification)

Created on 2022-06-01 by the reprex package (v2.0.1)

@simonpcouch
Copy link
Contributor Author

Thanks for the review, @hfrick! :) Ready to merge from our perspective, @topepo.

@topepo topepo merged commit 4473786 into main Jun 6, 2022
@topepo topepo deleted the print-refactor branch June 6, 2022 17:04
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants