search

econ-ark / HARK

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

IndShockConsumerType.calc_jacobian() changes cFunc_terminal_ #1421

Closed mnwhite closed 3 weeks ago

mnwhite commented 3 weeks ago

The calc_jacobian method improperly accesses and changes cFunc_terminal_, which is supposed to be a class object. I'm reworking things so that cFunc_terminal_ won't exist at all in the near future, but instances of a class shouldn't be changing objects defined at the class level. There is a way to do what calc_jacobian wants to do, but it went about it the wrong way.

This might be related to another issue we ran into lately, with a mysteriously needed duplicative call to update_solution_terminal() in order to pass tests for calc_Jacobian.

alanlujan91 commented 3 weeks ago

@wdu9 @Mv77 this might be relevant to you

Mv77 commented 3 weeks ago

I mean sure, its hacky.

But @wdu9 did it to get hark to do something it was not meant to do. It would be really annoying to have had to define a new class to do it properly.

The way I did it when I needed to do the same in my own branch was to add an optional extra argument to solve which would be a solution to serve as the start of backward induction. It makes it ignore the default terminal solution.

Should I make a PR with that change?

mnwhite commented 3 weeks ago

No, don't make any changes. There's another change that can be made only to the Jacobian stuff. I'll tackle it shortly.

The gist is to overwrite the local terminal cFunc, not the class-based one.

On Fri, Apr 26, 2024 at 1:48 PM Mateo Velásquez-Giraldo < @.***> wrote:

I mean sure, its hacky.

But @wdu9 https://github.com/wdu9 did it to get hark to do something it was not meant to do. It would be really annoying to have had to define a new class to do it properly.

The way I did it when I needed to do the same in my own branch was to add an optional extra argument to solve which would be a solution to serve as the start of backward induction. It makes it ignore the default terminal solution.

Should I make a PR with that change?

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

mnwhite commented 3 weeks ago

This is now fixed in #1410 by adding an optional boolean input to AgentType.solve, which overrides the usual call to presolve at the top of solve. The presolve routine does setup work, but in unusual situations a user might want to not do some of those things, like when they want to start with a "custom" terminal solution (as in calc_jacobian). With that in place, it's just a one line change in calc_jacobian to change how the "terminal solution hacking" works so that it doesn't touch something unintended.