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

Appending real time stats #692

Merged
merged 6 commits into from
Jun 5, 2023
Merged

Appending real time stats #692

merged 6 commits into from
Jun 5, 2023

Conversation

ijpulidos
Copy link
Contributor

@ijpulidos ijpulidos commented Apr 24, 2023

Description

Changing the default behavior of the multistate reporter to append in the YAML real time statistics file instead of overwriting.

We already had the machinery for this, we probably want to discuss if we want to expose this option to the user.

I tested this resuming a run, but we probably want to write a proper test for this behavior. WIP.

Resolves #691

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog message

Real time statistics in YAML file are now appended when resuming simulations, instead of being overwritten.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #692 (e862a22) into main (abb2f61) will decrease coverage by 0.26%.
The diff coverage is 66.66%.

@ijpulidos
Copy link
Contributor Author

A limitation that we have with this approach is that we would need the interval for writing the real time yaml files to be a multiple of the checkpoint interval, or else we would have duplicated iterations information when we resume simulations (think when we stop a simulation in the somewhere in middle of a checkpoint interval and we resume it, the real time yaml file will have duplicated iterations).

I guess we could add a check for this in the constructor of the MultiStateSampler object to avoid this issue.

@ijpulidos
Copy link
Contributor Author

Looking at the code probably the better API point where we should be creating this is in MultiStateSampler.create, rather than in the constructor. Since this is the place where the storage reporter is created as well, which is the one that handles the checkpointing.

@ijpulidos
Copy link
Contributor Author

This breaks quite a few tests that rely on not making any checkpoints but doing the online analysis. I wonder what's the best approach here.

@mikemhenry
Copy link
Contributor

I think we only need the check if we are writing checkpoints, right?

@ijpulidos
Copy link
Contributor Author

I think we only need the check if we are writing checkpoints, right?

Not sure exactly what you mean here, but we don't have a way to have no checkpointing at all, as far as I can tell. Tests just make the checkpoint_interval very large to avoid creating checkpoints.

@ijpulidos ijpulidos requested a review from mikemhenry May 25, 2023 19:19
@ijpulidos
Copy link
Contributor Author

Not sure exactly what you mean here, but we don't have a way to have no checkpointing at all, as far as I can tell. Tests just make the checkpoint_interval very large to avoid creating checkpoints.

We maybe want to have this capability in the future, maybe passing checkpoint_interval=None for the reporter to not generate any checkpoints at all.

@ijpulidos ijpulidos marked this pull request as ready for review June 5, 2023 18:25
@ijpulidos ijpulidos enabled auto-merge (squash) June 5, 2023 18:43
@mikemhenry mikemhenry disabled auto-merge June 5, 2023 20:24
@mikemhenry mikemhenry merged commit 30db3ab into main Jun 5, 2023
@mikemhenry mikemhenry deleted the 691-default-append-real-time branch June 5, 2023 20:24
@ijpulidos ijpulidos added this to the 0.22.2 milestone Jun 6, 2023
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.

Real-time analysis YAML files should append, not overwrite
2 participants