search

flatironinstitute / stan-playground

Run Stan models in the browser
Apache License 2.0
36 stars 1 forks source link

Clear analysis outputs when sampler is re-run #140

Closed WardBrian closed 4 months ago

WardBrian commented 4 months ago

This prevents having plots, etc that show draws from a previous run being left around in the UI

magland commented 4 months ago

Question about this. Does it clear it on the start of sampling or end? I think it could be useful to still view the old plots while the new sampling is running. Worth considering.

WardBrian commented 4 months ago

I made the condition for updating to be when latestRun.draws changes. At the moment, we do clear this as soon as sampling starts, but we wouldn't need to

WardBrian commented 4 months ago

Actually, I think clearing this at the start is the only robust option, because of this sequence of events:

  1. Run the sampler to completion with num_chains=4
  2. Change num_chains=6
  3. Run the sampler
  4. Run analysis.py before the sampler completes

3 has to set the SamplingOpts we store, which includes num_chains. But if it didn't clear draws, then 4 would be possible, but would end up giving in nonsense mismatched num_chains and the actual draws array to Python

magland commented 4 months ago

We could just disable running analysis.py if sampler is running. We already disable it for when no sampling is completed.

WardBrian commented 4 months ago

@magland my latest commit here makes the necessary changes to only clear the output when it is finished and to prevent the user running mid-sample

magland commented 4 months ago

This all looks good to me.

jsoules commented 4 months ago

I've no objections to the implementation, but I am a little on the fence about the right functionality. I appreciate that we might want to see the old plots/analysis during resampling, but it seems like it might be frustrating for that to all just disappear once the new sampling completes? (Which I believe we'd currently do, because it would be misleading to show stale analysis when new samples have come in)

I think the right behavior probably depends on how long we expect most sampling operations to take, no?

WardBrian commented 4 months ago

Which I believe we'd currently do, because it would be misleading to show stale analysis when new samples have come in

Correct, this is the basic thing the PR intends to prevent.

When they disappear would probably be equally frustrating for different users, but leaving them around while sampling seems more conservative. Of course, since we don't provide an "oops, undo that!" button, it's possible for you to be looking at the plots from your last run and realize you wish you had kept them, but that's a more general issue I think

WardBrian commented 4 months ago

Opinions on merging as an improvement over the status quo, even if we may revisit?

magland commented 4 months ago

I'm in favor.