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

Export S3 methods for model_performance generic #452 #498

Closed
wants to merge 4 commits into from
Closed

Export S3 methods for model_performance generic #452 #498

wants to merge 4 commits into from

Conversation

jimrothstein
Copy link

No description provided.

@IndrajeetPatil IndrajeetPatil linked an issue Oct 21, 2022 that may be closed by this pull request
@IndrajeetPatil
Copy link
Member

@jimrothstein Thanks a lot for doing this! I have modified it a bit to also update NAMESPACE.

@strengejacke I had to move the default method to where generic is defined because otherwise it won't be found by other methods that rely on the default method. The other solution would have been to use Collate field in DESCRIPTION, but I am not a fan of that option.

@codecov-commenter
Copy link

Codecov Report

Merging #498 (551580e) into main (ba99475) will increase coverage by 0.31%.
The diff coverage is 13.04%.

@@            Coverage Diff             @@
##             main     #498      +/-   ##
==========================================
+ Coverage   36.06%   36.37%   +0.31%     
==========================================
  Files          83       82       -1     
  Lines        5341     5341              
==========================================
+ Hits         1926     1943      +17     
+ Misses       3415     3398      -17     
Impacted Files Coverage Δ
R/model_performance.R 12.90% <13.04%> (+0.40%) ⬆️
R/model_performance.lm.R 73.95% <0.00%> (+1.04%) ⬆️
R/r2.R 37.45% <0.00%> (+5.99%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jimrothstein
Copy link
Author

@strengejacke yes, I should have checked where the .default method was defined.

@IndrajeetPatil
Copy link
Member

@jimrothstein Do you mind also updating the NEWS.md file and mention these changes?

Remember to include the PR number and your GitHub handle in the news item.

@jimrothstein
Copy link
Author

jimrothstein commented Oct 21, 2022

@IndrajeetPatil, Yes, will do, but tomorrow ... (was trying to find away to generate all the @export for the methods via embedded code using knitr ... did not work.)

@strengejacke
Copy link
Member

@strengejacke I had to move the default method to where generic is defined because otherwise it won't be found by other methods that rely on the default method. The other solution would have been to use Collate field in DESCRIPTION, but I am not a fan of that option.

Sounds good to me!

@jimrothstein jimrothstein closed this by deleting the head repository Jan 17, 2023
@IndrajeetPatil
Copy link
Member

@jimrothstein Why did you close this?

@strengejacke I think this can be reopened and merged?

@jimrothstein
Copy link
Author

@IndrajeetPatil @strengejacke

Hadn't heard from anyone in long time; and was cleaning repos.

Did not realize affect here, which is my fault, but I did not know.

@jimrothstein
Copy link
Author

@IndrajeetPatil @strengejacke

Do you want me to continue with tinytest:: checks ?

@strengejacke
Copy link
Member

Closing this for now, for two reasons:

  1. Export all S3 methods for model_parameters generic - 2nd attempt parameters#807 (comment)
  2. Not sure how to resolve conflicts with the command line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export more S3 methods for model_performance() generic
4 participants