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

Clear analysis outputs when sampler is re-run #140

Merged
merged 2 commits into from
Jul 24, 2024
Merged

Conversation

WardBrian
Copy link
Collaborator

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

@magland
Copy link
Collaborator

magland commented Jul 23, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

magland commented Jul 23, 2024

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

@WardBrian
Copy link
Collaborator Author

@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
Copy link
Collaborator

magland commented Jul 24, 2024

This all looks good to me.

@jsoules
Copy link
Collaborator

jsoules commented Jul 24, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@magland
Copy link
Collaborator

magland commented Jul 24, 2024

I'm in favor.

@WardBrian WardBrian merged commit 0d59626 into main Jul 24, 2024
2 checks passed
@WardBrian WardBrian deleted the clear-analysis-output branch July 24, 2024 20:49
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.

3 participants