-
Notifications
You must be signed in to change notification settings - Fork 20
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
add class: SimulationMacroCache #224
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #224 +/- ##
==========================================
- Coverage 82.56% 82.13% -0.43%
==========================================
Files 187 190 +3
Lines 9328 9574 +246
Branches 1183 1206 +23
==========================================
+ Hits 7702 7864 +162
- Misses 1348 1427 +79
- Partials 278 283 +5 ☔ View full report in Codecov by Sentry. |
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.
Thanks for your contributions to this @SylviaDu99. I've left a couple minor change requests, and it would also be appreciated if you could update your PR description. I'd also like to get Nikhil's review on this, so I've requested his review, as well.
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.
Thanks for this @SylviaDu99! Just a few comments.
from policyengine_core.taxbenefitsystems import TaxBenefitSystem | ||
|
||
|
||
class Singleton(type): |
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.
Sorry- could you explain why we need this?
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.
@SylviaDu99 Thanks again for this, apologies for the delay in reviewing. I requested just one minor change, then I think this should be good to merge.
""" | ||
if not self.is_over_dataset: | ||
return None | ||
if hasattr(self, "dataset"): |
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.
if hasattr(self, "dataset"): | |
if hasattr(self, "dataset") and self.dataset.data_format == Dataset.FLAT_FILE: |
I think this is better form instead of nesting if's
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.
Thanks for your hard work on this @SylviaDu99!
WIP: Another version for feat: SimulationMacroCache
targeting to fix: #197