Basic support for calling Pathfinder #19
Conversation
|
(The next thing I think I want to do in this interface is start breaking it up so that I have a Types.ts, Constants.ts, etc which separate out a lot of the boilerplate) |
|
Oh, also, Pathfinder doesn't really have a reasonable analogue of the progress for sampling. You tell it a maximum number of iterations to run, but in basically all practical problems it will run for less than that, so you can't really calculate a % complete. It's much faster than sampling though, so this is perhaps not as big an issue as it was for sampling, but maybe we will want a spinny wheel rather than a progress bar here. |
|
Is this something that would be run prior to sampling? I'm not familiar with pathfinder. Could you please give some explanation on how you imagine this working within stan-playground? |
|
I describe a bit in #15, but you can want to run Pathfinder for two different reasons. Eventually, I could see there being two buttons on the "samples" page for Pathfinder - one that downloads all of them as CSVs, and another that downloads a specified number of JSON files for initialization (or maybe it doesn't download them, maybe it just populates whatever initialization editor we end up coming up with). But the second thing is hard, so for now I think it is reasonable to just think of this as a strict alternative to sampling, e.g. a different tab altogether in the run panel |
|
Makes sense. Should we go ahead and merge this PR even though we are not giving the option of running pathfinder? ... and rebuild the docker? |
|
I think @jsoules wanted to take a look, but I do think we should merge this separately from the UI to use it just to keep it small |
| @@ -41,7 +41,7 @@ class StanSampler { | |||
| this.#onStatusChangedCallbacks.forEach(cb => cb()) | |||
| break; | |||
| } | |||
| case Replies.SampleReturn: { | |||
| case Replies.StanReturn: { | |||
There was a problem hiding this comment.
For my own clarification, what's the distinction being made here? Is it "pathfinder doesn't technically do sampling but will still use this verb so we don't want to call it Sample"?
There was a problem hiding this comment.
Yeah, whether or not you can still call the result of an approximation "draws" or "samples" is a bit sticky.
I also think the name previously was incorrect for when the return is actually an error
| return { paramNames, draws }; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Might want to convert the _malloc(4) calls below to use PTR_SIZE instead, since you added it.
There was a problem hiding this comment.
the 4 in those mallocs is a different 4 than the ones I changed here (it is not necessarily true that sizeof(int*) == sizeof(int), in fact on x64 systems it is false)
So I could define an INT_SIZE const variable, but those are also the only places I think the API will ever need to malloc an int (in retrospect, I could use Int32Array.BYTES_PER_ELEMENT as something more self-documenting, but also the fact that Int32 is in the name does pretty strongly imply the answer will be 4...)
Yeah, |
|
I'm going to merge this so I can build the new docker image and we can make sure everything is still working properly. |
|
Pushed new docker image, and everything seems to be working still with the public wasm compiler server, so that's good news. |
|
I built and tested the new image locally before opening the PR as well |
This adds the basics to allow calling Pathfinder:
_tinystan_pathfinderfunction in the build configRequestvariant in the worker for requesting Pathfinder be runThis this does not do:
Any UI for calling it instead of sampling (to test, I just replaced the call to the sampler with the call to pathfinder in our existing UI). It also has its own set of control flags we'd want to expose, like Control parameters for sampling #16
Any way to wire the results of Pathfinder up as initial values for sampling (needlessly hard, but probably worthwhile eventually)