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

Add information from model to fit #75

Closed
ahartikainen opened this issue Jun 2, 2019 · 11 comments
Closed

Add information from model to fit #75

ahartikainen opened this issue Jun 2, 2019 · 11 comments
Labels

Comments

@ahartikainen
Copy link
Contributor

Hi,

could we add model.program_code maybe other information too, so one can infer the model and recreate the model without model instance.

They could to dict under fit.model_info?

@riddell-stan
Copy link
Contributor

riddell-stan commented Jun 2, 2019 via email

@ahartikainen
Copy link
Contributor Author

True. Could we atleast add stancode? I don't know how many times I have pickled a fit without knowledge about the model (this is not an issue with pystan, where I pickle fit + model together).

This could also simplify the implementation in arviz.

@riddell-stan
Copy link
Contributor

How about storing a copy of model_name which is a hash of the program_code with the fit object. This would let us avoid storing duplicate information.

@ahartikainen
Copy link
Contributor Author

That could work, but fails when user incrementally updates model, overwriting the old model.

I don't think stan-code would create too much duplication, but it would increase the robustness of the fit object.

@riddell-stan
Copy link
Contributor

riddell-stan commented Jun 3, 2019

I have trouble imagining a scenario where I would lose track of my Stan program code. (I like the idea of storing the fit_name with the fit. This includes the model name (which has the hash of the stan program code).)

So here are my two arguments against adding the program code to the Fit object:

  1. It violates DRY; we already have the program_code attached to the Model instance. People should keep track of their Models. If they don't, that's their fault.
  2. It breaks the abstraction. The Fit is not the Model. The Model is the logical place for the program code.

If people are losing track of their Models, then this is a problem and we should find out a way to help them. I can imagine some sort of helper function which bundles Model and Fits together. We could add this helper function to the documentation just like we did with stan_cache with PyStan 2.

Edit: edits for clarity

@ahartikainen
Copy link
Contributor Author

  1. True that it violates DRY. But Model and Fit are separate objects. Users will still save only Fit, change models, forget models (reload Fit after many years; share Fit to coworker; make mistakes). Also Stan-code is small in size, doesn't lose its descriptiveness, "universal" being str.

  2. Sure we could make them to save together, but then what would be the point of having external Fit object.

Scikit-learn keeps model and fit in same class. I'm not saying we need to create hooks to Model object. Just that we could add it's stan code (maybe also include files, if they are txt), which enables one to wrap Fit object so user can recreate the model used to sample its data.

Also stan code is currently only way to parse dtypes for models, but I hope Stan3 fix this problem.

@riddell-stan
Copy link
Contributor

riddell-stan commented Jun 3, 2019 via email

@ahartikainen
Copy link
Contributor Author

Yes, this can be addressed after Stan3 lands.

I think my logic is that, given stan code is text / +#include texts, the inclusion would not add complexity, and would be easy way to increase the robustness of the fit (what is theta etc).

The current Stan2 fit... is a beast :) (was trying to add some minimal changes to VI to get log_p and log_g, but I guess the reader needs rewrite).

@riddell-stan
Copy link
Contributor

I definitely think it is a bug that there's no way right now to match Fit instance to Model instance. We definitely need to add fit_name as a field so people can at least look at the hashes. This discussion has brought that up.

There are so many ways to do this particular API. I'm not confident that this way is any better than, say, the way sklearn does it. All I know is that it is essentially what we settled on many years ago here: https://github.com/stan-dev/stan/wiki/User-Interface-Guidelines-for-Developers

Let's definitely revisit this in the future.

@ahartikainen
Copy link
Contributor Author

Is there any mechanism for this in RStan or CmdStan?

cc. @bgoodri @seantalts

@stale
Copy link

stale bot commented Oct 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 22, 2019
@stale stale bot closed this as completed Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants