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 all methods for model_parameters generic #805

Closed
wants to merge 3 commits into from
Closed

export all methods for model_parameters generic #805

wants to merge 3 commits into from

Conversation

jimrothstein
Copy link
Contributor

Can you take a peek?
Is formatting OK??

Will fix tomorrow... will have more time in a couple of days
(Thanks for the guidance ... it is appreciated)

jim

@IndrajeetPatil
Copy link
Member

Wow, thanks a ton, @jimrothstein!!

At a first glance, this looks good to me, but please give us some time to review this thoroughly.

Also, don't forget to update NEWS for logging this change and mention yourself.

@strengejacke
Copy link
Member

Thanks! I'll look into this when I'm back from my holiday.

@IndrajeetPatil
Copy link
Member

@jimrothstein In the meanwhile, if you are interested, you can also make the exact same change for the {performance} package: easystats/performance#452

@jimrothstein
Copy link
Contributor Author

jimrothstein commented Oct 20, 2022

@IndrajeetPatil ... yes, will do next: easystats/performance#452
Also would like to work through vignettes the way a regular user would. And practice a bit more git/github.

@jimrothstein
Copy link
Contributor Author

@IndrajeetPatil
Pushed NEWS.md for both {parameters} and {performance}, but fear I pushed wrong branch.
Will look carefully and fix tomorrow. I sometimes forget to think first.

@strengejacke
Copy link
Member

At a first glance, are you sure that these are all existing class names? Some look like package names and not valid classes.

@jimrothstein jimrothstein deleted the jim branch October 22, 2022 08:04
@jimrothstein
Copy link
Contributor Author

@strengejacke Can you give me an example or two non-classes?

My understanding that we want every S3 method (<generic>.<class>) in broom package to ALSO be listed in NAMESPACE file for parameter package. So for example S3 tidy.zoo is in broom, but wasn't listed in parameter.

ie compare (S3 methods only)

broom_tidy <- ls(getNamespace("broom"))

easystats_tidy <- ls(getNamespace("parameters"))

Did I misinterpret the problem?

@jimrothstein jimrothstein restored the jim branch October 22, 2022 08:58
@jimrothstein jimrothstein reopened this Oct 22, 2022
@strengejacke
Copy link
Member

My understanding that we want every S3 method (.) in broom package to ALSO be listed in NAMESPACE file for parameter package.

Yes, that is the intention, so your PR in general is ok.

Can you give me an example or two non-classes?

E.g., model_parameters.emmeans - I think emmeans only exists as package name, not as class.

@jimrothstein
Copy link
Contributor Author

Good catch!
package {broom} has a couple of functions of the form below, but are NOT S3 methods
tidy_X
tidy.X

This is insufficient.
broom_tidy <- broom_tidy[grepl("^tidy[.]", broom_tidy)]

Will work on more careful checks today.

@jimrothstein
Copy link
Contributor Author

jimrothstein commented Oct 24, 2022

@strengejacke

update: I think I found a better way than below. check a possible S3 method (tidy.XYZ) against methods("tidy")

Before I do another push, can you take quick look:

    • pattern="^tidy[.]" should catch imposters of the form tidy_X
    • sloop::is_s3_method() should be a better check, but I am not convinced it is doing what I think it is.
broom_tidy <- ls(getNamespace("broom"), pattern="^tidy[.]")
data.frame(method=broom_tidy, is_S3 = sapply(broom_tidy, sloop::is_s3_method, USE.NAMES=F))
                     method is_S3

1 tidy.aareg TRUE
2 tidy.acf TRUE
3 tidy.anova TRUE
4 tidy.aov TRUE
5 tidy.aovlist TRUE
6 tidy.Arima TRUE
7 tidy.betamfx TRUE
8 tidy.betareg TRUE
9 tidy.biglm TRUE
10 tidy.binDesign TRUE
11 tidy.binWidth TRUE
12 tidy.boot TRUE
13 tidy.btergm TRUE
14 tidy.cch TRUE
15 tidy.character TRUE
16 tidy.cld TRUE
17 tidy.clm TRUE
18 tidy.clmm TRUE
19 tidy.coeftest TRUE
20 tidy.confint.glht TRUE
21 tidy.confusionMatrix TRUE
22 tidy.coxph TRUE
23 tidy.crr TRUE
24 tidy.cv.glmnet TRUE
25 tidy.data.frame TRUE
26 tidy.default TRUE
27 tidy.density TRUE
28 tidy.dist TRUE
29 tidy.drc TRUE
30 tidy.durbinWatsonTest TRUE
31 tidy.emmGrid TRUE
32 tidy.epi.2by2 TRUE
33 tidy.ergm TRUE
34 tidy.factanal TRUE
35 tidy.felm TRUE
36 tidy.fitdistr TRUE
37 tidy.fixest TRUE
38 tidy.ftable TRUE
39 tidy.gam TRUE
40 tidy.Gam TRUE
41 tidy.garch TRUE
42 tidy.geeglm TRUE
43 tidy.glht TRUE
44 tidy.glm TRUE
45 tidy.glmnet TRUE
46 tidy.glmrob TRUE
47 tidy.glmRob TRUE
48 tidy.gmm TRUE
49 tidy.htest TRUE
50 tidy.ivreg TRUE
51 tidy.kappa TRUE
52 tidy.kde TRUE
53 tidy.Kendall TRUE
54 tidy.kmeans TRUE
55 tidy.lavaan TRUE
56 tidy.leveneTest TRUE
57 tidy.Line TRUE
58 tidy.Lines TRUE
59 tidy.list TRUE
60 tidy.lm TRUE
61 tidy.lm.beta TRUE
62 tidy.lmodel2 TRUE
63 tidy.lmrob TRUE
64 tidy.lmRob TRUE
65 tidy.logical TRUE
66 tidy.logitmfx TRUE
67 tidy.lsmobj TRUE
68 tidy.manova TRUE
69 tidy.map TRUE
70 tidy.margins TRUE
71 tidy.Mclust TRUE
72 tidy.mediate TRUE
73 tidy.mfx TRUE
74 tidy.mjoint TRUE
75 tidy.mle2 TRUE
76 tidy.mlm TRUE
77 tidy.mlogit TRUE
78 tidy.muhaz TRUE
79 tidy.multinom TRUE
80 tidy.negbin TRUE
81 tidy.negbinmfx TRUE
82 tidy.nlrq TRUE
83 tidy.nls TRUE
84 tidy.NULL TRUE
85 tidy.numeric TRUE
86 tidy.orcutt TRUE
87 tidy.pairwise.htest TRUE
88 tidy.pam TRUE
89 tidy.plm TRUE
90 tidy.poissonmfx TRUE
91 tidy.poLCA TRUE
92 tidy.polr TRUE
93 tidy.Polygon TRUE
94 tidy.Polygons TRUE
95 tidy.power.htest TRUE
96 tidy.prcomp TRUE
97 tidy.probitmfx TRUE
98 tidy.pyears TRUE
99 tidy.rcorr TRUE
100 tidy.ref.grid TRUE
101 tidy.regsubsets TRUE
102 tidy.ridgelm TRUE
103 tidy.rlm TRUE
104 tidy.rma TRUE
105 tidy.roc TRUE
106 tidy.rq TRUE
107 tidy.rqs TRUE
108 tidy.sarlm TRUE
109 tidy.Sarlm TRUE
110 tidy.SpatialLinesDataFrame TRUE
111 tidy.SpatialPolygons TRUE
112 tidy.SpatialPolygonsDataFrame TRUE
113 tidy.spec TRUE
114 tidy.speedglm TRUE
115 tidy.speedlm TRUE
116 tidy.summary_emm TRUE
117 tidy.summary.glht TRUE
118 tidy.summary.lm TRUE
119 tidy.summary.plm TRUE
120 tidy.summaryDefault TRUE
121 tidy.survdiff TRUE
122 tidy.survexp TRUE
123 tidy.survfit TRUE
124 tidy.survreg TRUE
125 tidy.svyglm TRUE
126 tidy.svyolr TRUE
127 tidy.systemfit TRUE
128 tidy.table TRUE
129 tidy.tobit TRUE
130 tidy.ts TRUE
131 tidy.TukeyHSD TRUE
132 tidy.varest TRUE
133 tidy.zoo TRUE

@jimrothstein
Copy link
Contributor Author

@IndrajeetPatil
Some local hardware issues ... Once I add code to double check export is for model_parameters S3 methods (only), should I delete this PR and begin a clean one?

@IndrajeetPatil
Copy link
Member

You can keep the current PR pending. Instead, create a new branch and open a new PR with a different implementation. If that is merged, then this one can be deleted.

Sounds reasonable?

@jimrothstein
Copy link
Contributor Author

jimrothstein commented Oct 26, 2022 via email

@IndrajeetPatil
Copy link
Member

Closing in favour of #807.

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.

3 participants