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

Reorganization of the predictor types #52

Open
DilumAluthge opened this issue Aug 19, 2020 · 6 comments
Open

Reorganization of the predictor types #52

DilumAluthge opened this issue Aug 19, 2020 · 6 comments

Comments

@DilumAluthge
Copy link
Collaborator

Currently, we only have a single predictor type:

struct SossMLJPredictor{M} <: Distributions.Distribution{MixedVariate, MixedSupport}
    model::M
    post
    pred
    args
end

I think we'll need a slightly more complicated set of types.

For predict_joint, we will continue to use SossMLJPredictor. But for predict, I think we'll need different types.

For predict with a classification model, we'll need to use MLJBase.UnivariateFinite for compliance with the MLJ model interface.

For predict with a regression model, it's not immediately obvious to me what we should return.

@cscherrer
Copy link
Owner

Yeah, I think you're right. How about this:

  • We keep SossMLJPredictor as the "general" case, which is probably also good for regression
  • Add SossMLJMarginalPredictor for marginals. I think users shouldn't have to type this, we can have a marginalize function to build it
  • MLJSossClassifier for classification in the MLJ sense, since that needs this special treatment

Oh, and we'll need type parameters on these, I just left it open to get a quick start

@DilumAluthge
Copy link
Collaborator Author

For consistency, can we name the last one SossMLJClassifier?

@cscherrer
Copy link
Owner

Oops, yes that's what I meant :)

@DilumAluthge DilumAluthge self-assigned this Aug 28, 2020
@DilumAluthge DilumAluthge modified the milestone: 0.1.0 Aug 28, 2020
@DilumAluthge
Copy link
Collaborator Author

DilumAluthge commented Aug 28, 2020

The parts of this that are blocking 0.1.0 are implemented in #91.

There is still additional work to be done, but it doesn't block 0.1.0.

@DilumAluthge DilumAluthge removed their assignment Aug 30, 2020
@cscherrer
Copy link
Owner

I'm now second-guessing whether we really need a SossMLJClassifier type. Let's see how it goes first

@DilumAluthge
Copy link
Collaborator Author

I'm going to mark this as potentially breaking, since it will probably result in some changes to the return types of public functions.

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

No branches or pull requests

2 participants