-
Notifications
You must be signed in to change notification settings - Fork 8
Add versions argument to Simulation
#148
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
Conversation
nikhilwoodruff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes:
- Drop support for Hugging Face in policyengine.py as discussed.
- Download the actual dataset version (after data package changes add this) using a new format:
data="gcs://bucket/[email protected]"
- When no version is specified, log the version of the dataset we used and warn the user that we're using this version.
- Added pip install prompt when model versions don't match.
|
@anth-volk added the changes from our in-person review just now. |
|
@anth-volk I also changed the version syntax because I think it's cleaner to have a standardised Old (this PR): |
anth-volk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving the work on this PR @nikhilwoodruff. Had a few thoughts and a couple questions in the comments. Additionally, wanted to ask if you could add relevant tests for new methods and one test to ensure that the function that adds the most recent version when its version arg is None does so properly.
|
@anth-volk addressed your comments, and added tests for the new functionality. |
anth-volk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor blocking issue: Optional-typed fields still need a default value to avoid validation errors when the field isn't passed. Otherwise, looking forward to getting this over the line today!
policyengine/outputs/macro/comparison/calculate_economy_comparison.py
Outdated
Show resolved
Hide resolved
policyengine/outputs/macro/comparison/calculate_economy_comparison.py
Outdated
Show resolved
Hide resolved
anth-volk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very excited to get this code in @nikhilwoodruff! It's been a pleasure reviewing.
Fixes #147