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.
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.
|
|
||
| def get_cache_value(self, cache_file_path: Path): | ||
| with h5py.File(cache_file_path, "r") as f: | ||
| # Validate both core version and country package metadata are uptodate, otherwise flush the cache |
There was a problem hiding this comment.
| # Validate both core version and country package metadata are uptodate, otherwise flush the cache | |
| # Validate both core version and country package metadata are up-to-date, otherwise flush the cache |
|
|
||
| variable = self.tax_benefit_system.get_variable(variable_name) | ||
| parameter_deps = variable.exhaustive_parameter_dependencies | ||
|
|
||
| if parameter_deps is None: | ||
| return None | ||
| return not is_cache_available |
There was a problem hiding this comment.
I think I would get rid of is_cache_available and just return either True or False in this function, as I think the is_cache_available value serves to complicate the function.
| from policyengine_core.taxbenefitsystems import TaxBenefitSystem | ||
|
|
||
|
|
||
| class Singleton(type): |
There was a problem hiding this comment.
Sorry- could you explain why we need this?
| if not self.is_over_dataset: | ||
| return None | ||
| is_cache_available = True | ||
| if self.is_over_dataset: |
There was a problem hiding this comment.
Since this PR started- we've added the ability to load DataFrames directly in simulations. Could we add an extra condition here that simulations from DataFrames are excluded from the macro cache? Thanks!
There was a problem hiding this comment.
@nikhilwoodruff Could you describe the syntax necessary for this?
There was a problem hiding this comment.
I take it it would involve using
if self.dataset.data_format == Dataset.TIME_PERIOD_ARRAYS:
?
There was a problem hiding this comment.
We need to check if the data format is Dataset.FLAT_FILE and disable the entire macro cache if yes.
| return None | ||
| is_cache_available = True | ||
| if self.is_over_dataset: | ||
| return is_cache_available |
There was a problem hiding this comment.
| return is_cache_available | |
| return True |
|
|
||
| variable = self.tax_benefit_system.get_variable(variable_name) | ||
| parameter_deps = variable.exhaustive_parameter_dependencies | ||
|
|
||
| if parameter_deps is None: | ||
| return None | ||
| return not is_cache_available |
There was a problem hiding this comment.
| return not is_cache_available | |
| return False |
| """ | ||
| if not self.is_over_dataset: | ||
| return None | ||
| is_cache_available = True |
There was a problem hiding this comment.
| is_cache_available = True |
| ) | ||
|
|
||
| return cache_file_path | ||
| return not is_cache_available |
There was a problem hiding this comment.
| return not is_cache_available | |
| return False |
| return None | ||
| with h5py.File(cache_file_path, "w") as f: | ||
| f.create_dataset("values", data=value) | ||
| return is_cache_available |
There was a problem hiding this comment.
| return is_cache_available | |
| return True |
WIP: Another version for feat: SimulationMacroCache
targeting to fix: #197