search

econ-ark / HARK

Heterogenous Agents Resources & toolKit
Apache License 2.0
315 stars 195 forks source link

Inheritance... #1406

Closed alanlujan91 closed 1 month ago

alanlujan91 commented 1 month ago

https://github.com/econ-ark/HARK/blob/3ee397993a1db76bbc9ae24d3196dc31ccfe4874/HARK/ConsumptionSaving/ConsIndShockModel.py#L3347

KinkedR inherits from IndShockConsumerType

https://github.com/econ-ark/HARK/blob/3ee397993a1db76bbc9ae24d3196dc31ccfe4874/HARK/ConsumptionSaving/ConsIndShockModel.py#L3369

But init's from PerfForesightConsumerType

is this a bug? does it make a difference?

mnwhite commented 1 month ago

I'd have to see if this is intentional or just a weird oversight. I don't think it's a bug, just an odd choice.

Init is something that should be standardized across AgentTypes. That's part of what I'm working on.

On Sat, Apr 6, 2024, 6:54 PM Alan Lujan @.***> wrote:

https://github.com/econ-ark/HARK/blob/3ee397993a1db76bbc9ae24d3196dc31ccfe4874/HARK/ConsumptionSaving/ConsIndShockModel.py#L3347

KinkedR inherits from IndShockConsumerType

https://github.com/econ-ark/HARK/blob/3ee397993a1db76bbc9ae24d3196dc31ccfe4874/HARK/ConsumptionSaving/ConsIndShockModel.py#L3369

But init's from PerfForesightConsumerType

is this a bug? does it make a difference?

— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1406, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFPVBYFXJF35CI4B37TY4B4JRAVCNFSM6AAAAABF2Y5G7WVHI2DSMVQWIX3LMV43ASLTON2WKOZSGIZDSNBVGE3DKMA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

alanlujan91 commented 1 month ago

I guess my question is why type the class name at all instead of super().__init__(**params) or for other methods across the codebase typing parent_class_name.method(self) instead of super().method(). It's not like we're doing a lot of double inheritance, and it would prevent these errors from happening since it always knows the right parent.

mnwhite commented 1 month ago

Oh, it might have been an oversight when programming it. Or there's some subtle reason to call PF's init rather than ConsIndShock-- I'm not sure. But either way, this is part of the "spaghetti inheritance" problem that we're unwinding.

On Sat, Apr 6, 2024, 7:06 PM Alan Lujan @.***> wrote:

I guess my question is why type the class name at all instead of super().init(**params) or for other methods across the codebase typing parent_class_name.method(self) instead of super().method(). It's not like we're doing a lot of double inheritance, and it would prevent these errors from happening since it always knows the right parent.

— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1406#issuecomment-2041227220, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFPAO4WIUEAUFH3K4PTY4B5V3AVCNFSM6AAAAABF2Y5G7WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRGIZDOMRSGA . You are receiving this because you commented.Message ID: @.***>

alanlujan91 commented 1 month ago

please see PR #1407

replaced references to parent class with super also removed some redundant calls to construction

there is one check failing which is concerning me, because I don't think I changed anything substantive, which makes me think there might have been a bug due to the spaghetti inheritance