Consolidate distribution classes #1592
Conversation
…tionality Co-authored-by: alanlujan91 <alanlujan91@gmail.com>
|
Cursor Agent can help with this pull request. Just |
Co-authored-by: alanlujan91 <alanlujan91@gmail.com>
Co-authored-by: alanlujan91 <alanlujan91@gmail.com>
Co-authored-by: alanlujan91 <alanlujan91@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates two distribution classes by merging TimeVaryingDiscreteDistribution functionality into IndexDistribution, simplifying the codebase and providing a unified API for handling indexed/time-varying distributions.
Key changes:
- Enhanced
IndexDistributionwith a newdistributionsparameter to directly wrap discrete distributions - Removed the redundant
TimeVaryingDiscreteDistributionclass - Updated all references and imports throughout the codebase to use the consolidated
IndexDistribution
Reviewed Changes
Copilot reviewed 6 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| HARK/distributions/base.py | Consolidated classes by adding distributions parameter to IndexDistribution and removing TimeVaryingDiscreteDistribution |
| HARK/distributions/init.py | Removed TimeVaryingDiscreteDistribution from exports |
| HARK/simulation/monte_carlo.py | Simplified type checking by removing TimeVaryingDiscreteDistribution references |
| HARK/distributions/utils.py | Updated utility function to work with consolidated IndexDistribution |
| HARK/core.py | Updated type checking to use only IndexDistribution |
| HARK/Calibration/Income/IncomeProcesses.py | Migrated TimeVaryingDiscreteDistribution usage to IndexDistribution |
| self.engine = engine | ||
|
|
||
| self.dstns = [] | ||
|
|
||
| # If no engine/conditional were provided, remain empty (should not happen in normal use) | ||
| if self.engine is None and not self.conditional: | ||
| return |
There was a problem hiding this comment.
This comment suggests a condition that 'should not happen in normal use', but the code allows it. Consider either removing this case handling or throwing an exception if this truly represents an invalid state.
| return | |
| # If no engine/conditional were provided, this is an invalid state. | |
| if self.engine is None and not self.conditional: | |
| raise ValueError("MarkovProcess: No engine or conditional parameters provided; this should not happen in normal use.") |
Copilot uses AI. Check for mistakes.
| @@ -367,70 +400,6 @@ def draw(self, condition): | |||
| these = c == condition | |||
| N = np.sum(these) | |||
|
|
|||
| cond = {key: val[c] for (key, val) in self.conditional.items()} | |||
| draws[these] = self[c].draw(N) | |||
|
|
|||
There was a problem hiding this comment.
The line cond = {key: val[c] for (key, val) in self.conditional.items()} was removed but self[c].draw(N) still depends on the conditional parameters being properly set for index c. This could cause incorrect behavior when drawing from indexed distributions.
Copilot uses AI. Check for mistakes.
Consolidate
TimeVaryingDiscreteDistributionintoIndexDistributionPlease ensure your pull request adheres to the following guidelines:
Why?
This PR unifies
IndexDistributionandTimeVaryingDiscreteDistributioninto a single, more versatileIndexDistributionclass. The previous existence of two distinct classes for handling indexed/time-varying distributions led to redundant code and an unnecessarily complex API. This consolidation simplifies the codebase by providing a single, consistent interface for both use cases.Changes:
IndexDistributionnow supports a newdistributionsparameter, allowing it to directly wrap a list of discrete distributions (absorbing the functionality ofTimeVaryingDiscreteDistribution).TimeVaryingDiscreteDistributionclass has been removed.IndexDistribution.