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

AbstractDynodeRunner code reorg and naming revamp #316

Merged
merged 16 commits into from
Jan 23, 2025

Conversation

arik-shurygin
Copy link
Collaborator

this PR does the following:

  • save_inference_timeseries now takes particle tuples as a list instead of randomly selecting, allowing for better user control over its behavior. The method will also return the saved dataframe instead of a str of where it was saved to. If user passes None to save_filename the csv will not be saved at all, in case that is required.
  • moves some functions hidden within the AbstractDynodeRunner out into utils so they may be more easily referenced by other code, as well as declutter the class itself.
  • Rename all references of timelines to timeseries when referring to the saving of metrics like vaccination or infection counts of a simulation.
  • Renaming the azure_visualizer_timelines.csv auto-generated file to simulation_timeseries.csv so it is easier to understand what it contains. Also the csv is used in much more than just the shiny app visualizers...

CLOSES #311
while this PR closes #311 some lasting software design decisions have made the request for parallelized loading of posterior particles not possible without substantial changes. I will create an issue #315 to track that

@arik-shurygin arik-shurygin requested a review from kokbent January 7, 2025 22:53
@arik-shurygin arik-shurygin marked this pull request as ready for review January 7, 2025 22:53
Copy link
Collaborator

@kokbent kokbent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked through vis_utils and part of save_inference_timeseries, will have to get back to it

examples/example_end_to_end_run.py Show resolved Hide resolved
src/dynode/dynode_runner.py Show resolved Hide resolved
src/dynode/dynode_runner.py Outdated Show resolved Hide resolved
src/dynode/dynode_runner.py Outdated Show resolved Hide resolved
src/dynode/dynode_runner.py Show resolved Hide resolved
src/dynode/mechanistic_runner.py Show resolved Hide resolved
src/dynode/utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@kokbent kokbent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked about renaming functions in call, implement and good to gooo!!!

@arik-shurygin arik-shurygin merged commit 149fbb1 into main Jan 23, 2025
4 checks passed
@arik-shurygin arik-shurygin deleted the generate-timeseries branch January 23, 2025 20:39
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.

Expand AbstractDynodeRunner.save_inference_timelines()?
2 participants