melff / mclogit

mclogit: Multinomial Logit Models, with or without Random Effects or Overdispersion
http://melff.github.io/mclogit/
22 stars 4 forks source link

Add emmeans support #18

Closed rvlenth closed 3 years ago

rvlenth commented 3 years ago

Hello,

This PR adds methods so that the emmeans package may be used with mblogit and mmblogit objects. Actually, that package contains its own preliminary support, and this PR duplicates that support. Any changes you make will override the internal emmeans methods.

I also provided an example under ? "emmeans-support".

It is better if you incorporate this code in your own package because you understand the structure of your model objects, and I may very well have overlooked something. I already know that, nominally, these methods also seem to run for mclogit objects, but I'm not sure it is correct or complete. I am more than happy to answer any questions

You may also wish to take a look at a user request https://github.com/rvlenth/emmeans/issues/268

melff commented 3 years ago

Dear Professor Lenth,

Thanks for your interest in adding "emmeans" support to mclogit. While this is certainly worthwhile in principle, I am not sure that it the best approach to add this support to the mclogit package rather than the emmeans package, given the usual way R-packages build on each other on CRAN. Your suggestion involves a manipulation of package namespaces that the CRAN people tend to frown upon. A more natural approach would be if the emmeans package provides the relevant S3 methods for "mclogit" and "mblogit" packages. If that requires an extension of the API of the mclogit package, I would be happy to implement it.

Best, Martin Elff

rvlenth commented 3 years ago

Thanks for your comments. I see your point about manipulating namespaces. I did that because it saves the amount of code that would be needed. But you are right that that is not the ideal way to do it.

The emmeans package supports quite a number of other packages, and occasionally, is in the case of mclogit, I receive user requests to support yet more. I have occasionally experienced cases where a change is made in one of those other packages that breaks my emmeans interface. Moreover, I really don't always know what capabilities exist for the models created by those packages. Accordingly, I have found it more desirable to work with package developers to have them put an emmeans interface in their package. The emm_basis method is essentially like the inside of a predict method for new data.

I am sure I would be able to develop a new PR for this purpose that does not require that objectionable namespace manipulation. If I did that, would you be interested? Or are you firm on not wanting an emmeans interface in your package?

melff commented 3 years ago

I think that it is best to avoid circular depencies between packages that provide estimators and packages that provide ways to compute quantities of interest from estimators, because it makes it difficult to maintain the packages involved. So I would prefer that there is an "emmeans" method for "mclogit" objects in the "emmeans" package, because I think you as the author have a much better understanding of what is required for this. That notwithstanding I'd be happy to accept a PR that provides S3methods that are exported from the namespace in the "standard" way by \S3method{...,mclogit} and that works without get(..., asNamespace("emmeans")) and

if (requireNamespace("emmeans", quietly = TRUE))
    emmeans::.emm_register(c("mblogit"), pkg)

In that case mclogit would "enhance" emmeans without implicitly of importing from it. That would avoid (as far as I understand it) a circular dependency.

Perhaps it would be possible to add

if (requireNamespace("mclogit", quietly = TRUE))
    emmeans::.emm_register(c("mblogit"), pkg)

to the emmeans package?

rvlenth commented 3 years ago

I'll take your answer as "no".

I think you misunderstand the .emm_register() function. First, I am pretty sure you would not want \S3method{emm_basis, mclogit} as part of your package code once you understand that that would require mclogit to import emmeans so it has the generic function emmeans::emm_basis -- and also requiring all of your package's users to install the emmeans package. The code you want me to remove registers the methods conditionally, so that if a user does not have emmeans installed, the methods are not registered and life goes on without that interface to my package. There are several packages that use this kind of conditional registration. And CRAN does not disapprove of it; just the opposite, because it reduces dependencies on other packages.

On the flip side, for classes with emm_basis and recover_data methods in the emmeans package, there is no need to register or export them per your final suggestion, because the methods already exist in emmeans's namespace, and hence will be found and used by the emmeans() function and relatives which run in the same namespace.

And there would be no kind of circular dependency if you had a free-standing emm_basis method that is conditionally registered.

melff commented 3 years ago

I was of the impression that it is possible to provide methods with S3method() without needing to import the generic. But in fact "Writing R Extensions" now tells me otherwise. So you are obviously right. I am reopenig your PR and perform a --as-cran check.

rvlenth commented 3 years ago

I made everything stand-alone by copying-over and modifying the parts of the multinom methods I was using. There are a couple of ulilities exported by emmeans that are explicitly given (hence no need to import anything from emmeans. This code will be run only if called by emmeans, so this is not an issue. But I did add two packages used to Suggests.

melff commented 3 years ago

Thanks for this, this starts to look really convincing. I guess I am going to merge this. But maybe I should give you another day or so, just in case you come up with further improvements. Or do you think your PR is finished now?

All the best, Martin Elff

rvlenth commented 3 years ago

Martin,

I think I'm done, in that I have managed to duplicate what I have for multinom models, and obtain exactly the same results for that housing example. This also works, I'm pretty sure, if there is a random component in the model -- but I don't have an example of that that I can successfully fit beyond the one a user provided in https://github.com/rvlenth/emmeans/issues/268 (which I tested and it "works" at least in the sense that it produces plausible results).

You were exactly right to call what I contributed into question. The original PR farmed stuff out to my multinom methods; and one byproduct of that was hiding from you some important aspects of what's going on. For example, the stuff with the Kronecker product to construct the needed linear functions for the multinomial response. Now all this is in front of you, making it more possible for you to maintain it.

Where I have few ideas, but you may have some, how to support mclogit models. Skimming through some of your documentation I can see in principle what is going on, but not clearly how to estimate some kind of probability that a subject will make a particular choice, or some kind of desirability measures for the available choices (comparable to the "latent" mode. That is where you have the expertise, and I hope this helps foster some ideas.

Russ

melff commented 3 years ago

Merged into master, thanks!

rvlenth commented 3 years ago

Great -- I'll update my models vignette to say the interface is in your package.

I've been thinking about mclogit objects a bit more. The existing interface for lm seems to work, due to inheritance. My thinking now is that that may be all that is needed. Perhaps the different choice sets are like incomplete blocks. Anyway, I'll be interested in seeing if you expand that support.