Skip to content

Vi summary #2230

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

Merged
merged 5 commits into from
May 28, 2017
Merged

Vi summary #2230

merged 5 commits into from
May 28, 2017

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented May 27, 2017

Here I propose a simple thing to record arbitrary summary during variational inference. I considered not to wrap VI result yet as it makes code messy, less readable and developer friendly

@junpenglao
Copy link
Member

This is a nice idea.

@fonnesbeck
Copy link
Member

I dont know if we want users having to set up explicit callbacks. Something more automated might be better; what about adding attributes to the approximation?

@fonnesbeck
Copy link
Member

fonnesbeck commented May 27, 2017

Along those lines, I do like the sort of comprehensive summary tables that you get when running statsmodels. Maybe having a summary=True argument on fit could provide a default set of statistics, with an optional dict for custom summaries?

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

Maybe, maybe. So it seems like it will be really convenient to have such a property

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

From another view I don't think that we can give user something more usefull than ELBO history.

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

I think that approx.hist introduced by @twiecki is enough for most of our users.
I assume that VI is used for complex tasks and users are mostly experienced enough. Moreover when using VI you usually need more that one inference.fit run as you track convergence(convergence callbacks can perform badly sometimes). That causes some issues with storing previous results. There are a lot of week places. For example if I use another stats, what will happen with my old ones? Should they disappear or filled with keys of new stats in addition? If so what will happen with consistence of enumeration? That can be pain. That's why I think users who use VI will find this api more handy.

@twiecki
Copy link
Member

twiecki commented May 27, 2017

I do like @fonnesbeck's idea of an Approximation.summary(). This PR does not rule out that approach of course, so it is a pretty simple code change for major flexibility.

@ferrine what other variables do you think users would track?

I'm fine with this, even though I'm not a big fan of callbacks I don't have a better idea.

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

Sometimes I track parameter difference, sometimes parameters themselves, sometimes it is an arbitrary function that depends on approximation (asvgd experiments). Generally I can't know in advance what I'll be interested in. All that stuff were one line functions but supplementary code was like a boilerplate. According to my experience I considered that this api fits all my needs. This Summary class is more like a util callback and better be moved to callbacks rather than inference.
As for static summary this can be a good idea if there is a need for report. I have no idea of that else I'd like to get in addition to elbo hist.
PS keep in mind that tracking can be memory consuming. That's another reason why I avoid storing additional things in Approximation

@twiecki
Copy link
Member

twiecki commented May 27, 2017

OK, that makes sense. Want to write a short test?

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

Sure

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

Tracker might be a better name for this callback

@ferrine
Copy link
Member Author

ferrine commented May 27, 2017

Are there any restrictions for merge because of upcoming 3.1?

@twiecki
Copy link
Member

twiecki commented May 27, 2017

Not yet. I like Tracker.

@twiecki
Copy link
Member

twiecki commented May 27, 2017

Other name ideas are Monitor , Recorder .

@ferrine
Copy link
Member Author

ferrine commented May 28, 2017

Tracker sounds better:)

@ferrine ferrine merged commit c3afa00 into pymc-devs:master May 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants