Add new create_inits() methods to other stanfit classes
#791
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #791 +/- ##
===========================================
+ Coverage 80.24% 80.60% +0.35%
===========================================
Files 25 25
Lines 3878 3949 +71
===========================================
+ Hits 3112 3183 +71
Misses 766 766 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks! Overall this looks great, I just have a few minor comments.
Re:
I assume this is already well-known but it seems like there is a cleaner library structure to have each of these different stanfit objects be implementations of the same parent (or abstract) class to unify these implementations (and those of other common methods).
Yes, we definitely could. So far most of what we've done in this style is using helper methods we throw in the utils subpackage for code re-use, but inheritance would also get us there, and maybe a bit more cleanly. In another project that deals with Stan outputs, I don't even differentiate between the different algorithms, I just have one 'StanOutput' class that can hold any of them.
cmdstanpy/stanfit/mle.py
Outdated
| if chains == 1: | ||
| return mle_inits | ||
| else: | ||
| return [mle_inits for _ in range(chains)] |
There was a problem hiding this comment.
Because the default behavior is if you pass a dictionary to sample, it uses it for every chain, I don't know if this is super necessary.
I do think this function should probably accept both a seed and chains argument, even if they're both unused, to let people write more generic code
There was a problem hiding this comment.
(I also think this function might be exactly equivalent to self.stan_variables() after those changes?
There was a problem hiding this comment.
Updated the signature for compatibility. I also use stan_variables() in the function now, but because stan_variables() will currently return a float if the parameter is scalar, I just map everything to an np.array for type compatibility. It seems this is intended to be the future behavior of stan_variables() in 2.0, at which point it will be equivalent.
This change simplifies the create_inits() for the MLE params to always return a dictionary of inits, rather than a list for multiple chains. The default behavior of the sample method is to initialize all chains at the same init if only one is given per param. The chains paramter is kept and the seed parameter is added to the signature, despite being no-ops, for the purposes of maintaining uniformity across the other create_inits() methods on other stanfit objects.
|
Thanks for the review -- responded to your comments and made changes where appropriate. Not sure what's going on with that failed pytest check. Some of the cancelled jobs seem to have gotten through the testing stage without issue. Going through the library, there are a number of references to a 2.0 version -- that might be a good opportunity to clean up some of these stanfit objects and such. Not sure if there are explicit plans for what that will contain (or timeline), just a thought. Happy to help out if there's a direction there. |
|
Because ADVI can be unstable it might be worth setting a known-good seed in the new test that runs it |
|
Okay, pinned some seeds that seemed fine and checks look good. Serendipitous that I went back and did that, because I found a bug in one of the tests I wrote, which I now have fixed. I think this should be set? |
|
Thanks. @mitzimorris may be able to review and approve sooner than @WardBrian for this one. |
Submission Checklist
Summary
This PR addresses #745. Implementation largely mirrors that for
CmdStanPathfinder. In particular, this adds the following methods:CmdStanVB.create_inits(),CmdStanLaplace.create_inits(),CmdStanMLE.create_intis(),CmdStanMCMC.create_inits(). With the exception of MLE, inits are selected by sampling without replacement from draws. The MLE implementation will initialize all requested chains at the optimized parameter values.Unit tests for all these methods are added as well.
I also updated the User's Guide example on creating inits from VI outputs to be more general and cover these new methods.
I assume this is already well-known but it seems like there is a cleaner library structure to have each of these different stanfit objects be implementations of the same parent (or abstract) class to unify these implementations (and those of other common methods). I haven't worked through the details, but it seemed possible given how similar the internal structures tend to be.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): myself
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: