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.

Sign up for GitHub

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

Jump to bottom

Revise basic SIR tutorial #377

Merged
merged 46 commits into from
Dec 7, 2023
Merged

Revise basic SIR tutorial #377

merged 46 commits into from
Dec 7, 2023

Conversation

SamWitty
Copy link
Collaborator

@SamWitty SamWitty commented Nov 13, 2023 •
edited
Loading

This PR revises the basic example as follows:

  1. Adds textual content, addressing Dynamical systems tutorial is missing textual content #343.
  2. Changes the plots to be more short-term focussed, satisfying the request of DARPA ASKEM program manager.
  3. Removes the counterfactual reasoning with dynamical systems, as discussed in Consider splitting up dynamical system tutorial #344 and subsumed by Notebook demonstrating counterfactual inference with dynamical systems #376.
  4. Suppresses deprecated usage warnings from seaborn
  5. Adds the notebook (with smoke test parameters) to the CI partially addressing Add missing notebooks to notebook CI stage #410

@SamWitty SamWitty added examples status:WIP Work-in-progress not yet ready for review labels Nov 13, 2023
@SamWitty SamWitty self-assigned this Nov 13, 2023
@SamWitty
Merge branch 'master' of https://github.com/BasisResearch/causal_pyro …
5690500 
…into sw-revise-SIR-tutorial
@SamWitty
remove unused imports and reorder imports
7d21af6
@SamWitty
add failing LogTrajectory test
f5b6d24
@SamWitty
revise tests to exercise start and end time collisions
9fd7c52
@SamWitty
removed unnecessary imports from test
6b3f985
@SamWitty
Merge branch 'master' of https://github.com/BasisResearch/causal_pyro …
1cedd1e 
…into sw-time-collision
@SamWitty
Merge branch 'master' of https://github.com/BasisResearch/causal_pyro …
b479d32 
…into sw-time-collision
@SamWitty
much simpler implementation
dc69b91
@SamWitty
lint and comment
1abc181
@SamWitty
added some functional indirection to appease linter
ae4d013
@SamWitty
lint
2693668
@SamWitty
type refinement
9c2e936
@SamWitty
nit about arg unpacking order
f513a0e
@SamWitty
Merge branch 'master' of https://github.com/BasisResearch/causal_pyro …
053d047 
…into sw-time-collision
@SamWitty
Merge branch 'sw-time-collision' of https://github.com/BasisResearch/…
fe666b9 
…causal_pyro into sw-time-collision
@SamWitty
added multiple simulate handling
ff0a626
@SamWitty
remove commented stop
1b84677
@SamWitty
lint
6b20372
@SamWitty
made BatchObservation handler use a continuation to guarantee it's ap…
f2132ce 
…plied after all solves
@SamWitty
lint
ce605a0
@SamWitty SamWitty mentioned this pull request Dec 4, 2023
Merged
@SamWitty SamWitty added status:awaiting review Awaiting response from reviewer and removed status:WIP Work-in-progress not yet ready for review labels Dec 6, 2023
eb8680
eb8680 reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

Looks much better! Two comments:

  • Can you remove the .diff method indirection in the models following Use functional interface for dynamical systems #341?
  • Should we consider moving the pyro.deterministic calls in helper functions like simulated_bayesian_sir that expose the trajectory to Predictive into our LogTrajectory handler?

@eb8680 eb8680 added status:awaiting response Awaiting response from creator and removed status:awaiting review Awaiting response from reviewer labels Dec 6, 2023
Base automatically changed from sw-time-collision to master December 7, 2023 02:17
@SamWitty SamWitty removed the blocked label Dec 7, 2023
@SamWitty SamWitty mentioned this pull request Dec 7, 2023
Open
5 tasks
@SamWitty SamWitty added status:awaiting review Awaiting response from reviewer and removed status:awaiting response Awaiting response from creator labels Dec 7, 2023
@SamWitty SamWitty requested a review from eb8680 December 7, 2023 13:47
@SamWitty
Copy link
Collaborator Author

SamWitty commented Dec 7, 2023

Looks much better! Two comments:

  • Can you remove the .diff method indirection in the models following Use functional interface for dynamical systems #341?
  • Should we consider moving the pyro.deterministic calls in helper functions like simulated_bayesian_sir that expose the trajectory to Predictive into our LogTrajectory handler?

@eb8680 , I believe these are now addressed.

@eb8680 eb8680 merged commit 08ac1bf into master Dec 7, 2023
7 checks passed
@eb8680 eb8680 deleted the sw-revise-SIR-tutorial branch December 7, 2023 14:02
This was referenced Dec 7, 2023
Closed
Merged
SamWitty added a commit that referenced this pull request Dec 8, 2023
@azane @SamWitty @eb8680
Fast-Forward Staging to Master (#450)
dba50c8 
* Revise basic SIR tutorial (#377)

* changes dynamical systems intro to be more short-term focussed

* suppress warnings

* remove counterfactual demo

* remove unused imports and reorder imports

* add failing LogTrajectory test

* revise tests to exercise start and end time collisions

* removed unnecessary imports from test

* much simpler implementation

* lint and comment

* added some functional indirection to appease linter

* lint

* type refinement

* nit about arg unpacking order

* added multiple simulate handling

* remove commented stop

* lint

* made BatchObservation handler use a continuation to guarantee it's applied after all solves

* lint

* add dynamical intro notebook to CI build

* add CI test parameters

* add a bunch of textual content

* Added description of example

* fixed bug in inference and added some plot changes

* fixed bug in inference and added some plot changes

* updated plots

* updated plots

* more plot updates

* more plot updates

* finished first pass of text, will edit tomorrow morning

* reran and minor edits

* text edits

* add glossary

* Update solver.py

missed merge conflict

* previous commit fix

* add dynamical systems dependencies to CI workflow

* addressing comments

* reran notebook

* Mention dynamical systems module in README (#435)

* Make State into a type alias of Mapping (#433)

* Make State a Mapping

* fix construction

* add dynamical to test requirements (#434)

* Fix header formatting in dynamical_intro (#436)

* Fix header formatting in dynamical_intro

* fix bullet spacing

---------

Co-authored-by: Sam Witty <[email protected]>

---------

Co-authored-by: Sam Witty <[email protected]>
Co-authored-by: eb8680 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eb8680 eb8680 eb8680 approved these changes

Assignees

@SamWitty SamWitty

Labels
examples module:dynamical status:awaiting review Awaiting response from reviewer
Projects
None yet
Milestone
Cleanup after ASKEM November 2023 hac...
Development

Successfully merging this pull request may close these issues.
2 participants
@SamWitty @eb8680