Open alanlujan91 opened 1 year ago
I agree it should be more transparent, but I don't think it can be automatically or endogenously transparent. That is, we can put it in the documentation, but having the code itself try to generate a description would be more work than it's worth.
The distance metric is labyrinthine, but easy for the user to implement when making new classes whose "distance" might be computed: all they need to do is name the relevant attributes, nothing else. Because our solution representations are complicated / have many nested layers, the computational description can be insanely complicated, even though the human description is intuitive and simple.
On Fri, Aug 4, 2023 at 4:47 PM Alan Lujan @.***> wrote:
We need to make it much more transparent how we are comparing the distances between our solutions and solution objects. Currently the way to do it is very labyrinthian and to figure it out you have to know how to follow a long rabbit hole.
We should be able to specify clearly what the distance criteria is and the user should be able to understand how that is happening readily.
@llorracc https://github.com/llorracc
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFPO2N636SRBGXHDOK3XTVNWFANCNFSM6AAAAAA3EVWDLI . You are receiving this because you are subscribed to this thread.Message ID: @.***>
That's not good enough. Convergence criteria are vital and Decory couldn't really figure out how to drill down to figure out what they were on our basic ConsIndShockModel. With me helping, it took 10 minutes. At a minimum, there should be better breadcrumbs, which shouldn't be too hard.
For infinite horizon ConsIndShock models, we should provide at least three options. One should be the change in MNrmStE, one should be the change in MNrmTrg, and one should be the current default (largest gap at our arbitrary gridpoints).
On Fri, Aug 4, 2023 at 4:57 PM Matthew N. White @.***> wrote:
I agree it should be more transparent, but I don't think it can be automatically or endogenously transparent. That is, we can put it in the documentation, but having the code itself try to generate a description would be more work than it's worth.
The distance metric is labyrinthine, but easy for the user to implement when making new classes whose "distance" might be computed: all they need to do is name the relevant attributes, nothing else. Because our solution representations are complicated / have many nested layers, the computational description can be insanely complicated, even though the human description is intuitive and simple.
On Fri, Aug 4, 2023 at 4:47 PM Alan Lujan @.***> wrote:
We need to make it much more transparent how we are comparing the distances between our solutions and solution objects. Currently the way to do it is very labyrinthian and to figure it out you have to know how to follow a long rabbit hole.
We should be able to specify clearly what the distance criteria is and the user should be able to understand how that is happening readily.
@llorracc https://github.com/llorracc
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ADKRAFPO2N636SRBGXHDOK3XTVNWFANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you are subscribed to this thread.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1666173105, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKCK77TQS4LFZQIAC2R2S3XTVO5FANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: @.***>
--
I'm not sure what "that's not good enough" is directed at. If a user knows about distance_criteria and has solved a one period version of the problem, they can dig down pretty easily. My prior post about transparency meant that the existence of distance_criteria might not be well advertised in the documentation, and I know that convergence criteria aren't mentioned in model descriptions.
When I first wrote HARK, I was imagining all of the various model files living in the user's working project directory, so that they could make changes on the fly. This was before we were on GitHub, and way before we had a distributable package. Under that framework, it is very, very easy to change convergence criteria: just change one list of strings! In modern HARK, it's not easy at all (unless you're working in developer mode). It should be something that can be changed at the level of an AgentType instance, but instead is bound to solution classes themselves.
On Sun, Aug 6, 2023, 11:10 PM Christopher Llorracc Carroll < @.***> wrote:
That's not good enough. Convergence criteria are vital and Decory couldn't really figure out how to drill down to figure out what they were on our basic ConsIndShockModel. With me helping, it took 10 minutes. At a minimum, there should be better breadcrumbs, which shouldn't be too hard.
For infinite horizon ConsIndShock models, we should provide at least three options. One should be the change in MNrmStE, one should be the change in MNrmTrg, and one should be the current default (largest gap at our arbitrary gridpoints).
On Fri, Aug 4, 2023 at 4:57 PM Matthew N. White @.***> wrote:
I agree it should be more transparent, but I don't think it can be automatically or endogenously transparent. That is, we can put it in the documentation, but having the code itself try to generate a description would be more work than it's worth.
The distance metric is labyrinthine, but easy for the user to implement when making new classes whose "distance" might be computed: all they need to do is name the relevant attributes, nothing else. Because our solution representations are complicated / have many nested layers, the computational description can be insanely complicated, even though the human description is intuitive and simple.
On Fri, Aug 4, 2023 at 4:47 PM Alan Lujan @.***> wrote:
We need to make it much more transparent how we are comparing the distances between our solutions and solution objects. Currently the way to do it is very labyrinthian and to figure it out you have to know how to follow a long rabbit hole.
We should be able to specify clearly what the distance criteria is and the user should be able to understand how that is happening readily.
@llorracc https://github.com/llorracc
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331, or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ADKRAFPO2N636SRBGXHDOK3XTVNWFANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you are subscribed to this thread.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1666173105, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAKCK77TQS4LFZQIAC2R2S3XTVO5FANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1667120483, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFIYJKKIKRTNNL2PG2LXUBMDFANCNFSM6AAAAAA3EVWDLI . You are receiving this because you commented.Message ID: @.***>
Just to chime in: I am a big fan of how we compute distances. In particular, I love the fact that the distance function is recursive (maybe that makes it confusing?). We are handling complex objects and I find the recursive distance solution clever and pythonic in that it is object-oriented and uses class inheritances.
The distance metric definition for consumer solutions in IndShockConsumer is the first line in their class definitions https://github.com/econ-ark/HARK/blob/4f644bf1a22362ae5fd1b20570ecad4074456edb/HARK/ConsumptionSaving/ConsIndShockModel.py#L144
Maybe, as @mnwhite says this is just an advertisement and documentation issue? It might be good to have metrics feature more prominently in the docs, and also print what object is being compared by the solve method?
If we wanted to flexibilize it, a way could be to let objects have a list of properties that could plausibly be used in the distance metric (e.g., [vPFunc, vFunc, MNrmStE]
) and then have the solver receive an optional parameter indicating what subset of those to use, with reasonable defaults.
I think one compromise is that we can define the class method in-place
to overwrite the default recursive look up.
class ConsumerSolution(MetricObject):
....
def distance(self, other):
return custom_distance
This should overwrite the default case when implemented.
That sounds fine to me. The other thing we can do is add a method (to MetricObject or whatever it's called) to have it describe the recursion. All you would need is an instance of whatever class you're interested in, and the method would (essentially) compare the instance to itself, using the same recursion as the distance function. Rather than take differences (which we know would be zero), it would construct a string output that describes the "layers" it's going through, and whether any non-trivial comparison is happening there.
It's fine if someone wants to use mNrmTrg or mNrmStE as their distance criteria, but it opens up something of a can of worms. These objects don't properly exist at the one-period-solver level, especially under our planned compartmentalization of the problem / stages. They only have any meaning when the problem actually is a chained sequence of consumption-saving problems, and that's not something the one period solver is supposed to know about. These methods should live at the AgentType (subclass) level, not at the solver level.
On top of that, computing these values every period is cripplingly slow. The basic ConsIndShock solver is currently spending 70% or more of its time on this (often pointless) task, at the default solver values.
On Tue, Aug 8, 2023 at 9:26 AM Mateo Velásquez-Giraldo < @.***> wrote:
Just to chime in: I am a big fan of how we compute distances. In particular, I love the fact that the distance function is recursive (maybe that makes it confusing?). We are handling complex objects and I find the recursive distance solution clever and pythonic in that it is object-oriented and uses class inheritances.
The distance metric definition for consumer solutions in IndShockConsumer is the first line in their class definitions
Maybe, as @mnwhite https://github.com/mnwhite says this is just an advertisement and documentation issue? It might be good to have metrics feature more prominently in the docs, and also print what object is being compared by the solve method?
If we wanted to flexibilize it, a way could be to let objects have a list of properties that could plausibly be used in the distance metric (e.g., [vPFunc, vFunc, MNrmStE]) and then have the solver receive an optional parameter indicating what subset of those to use, with reasonable defaults.
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669613755, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFJQEG7M7RKBHFF3YCLXUI5ANANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: @.***>
I’d be quite satisfied with a fix in which:
PS. I had no idea that computing MNrmTrg and MNrmStE were as expensive as you say — consuming 70 percent of the compute time for each iteration (really? did I get that right?). Maybe another alternative would be to set things up so that there could be a crude metric (say, the default) and a refined metric that gets computed optionally, and only once the crude metric is satisfied.
In any case, for reasons I will explain when your mini-family returns from its vacation, this is actually a high priority at the moment. (cc’ing Decory because it’s high priority derives from things he is working on).
On Tue, Aug 8, 2023 at 10:07 AM Alan Lujan @. @.> wrote:
I think one compromise is that we can define the class method in-place to
overwrite the default recursive look up.
class ConsumerSolution(MetricObject):
.... def distance(self, other): return custom_distance
This should overwrite the default case when implemented.
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669691494, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKCK7ZLKBWHFJDMQZO73A3XUJB2JANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: <econ-ark/HARK/issues/1331/1669691494 @.***>
--
- Chris Carroll
You're correct, under the default parameters, 70% of computation time is spent on calc_stable_points. If you crank up the number of nodes in the income distribution (so that integration is very expensive), that balance changes.
It costs a lot because it's a sequential or iterative search, one value at a time, as it zooms in to find the targets.
On Tue, Aug 8, 2023, 11:48 AM Christopher Llorracc Carroll < @.***> wrote:
I’d be quite satisfied with a fix in which:
- The user can pass an option to the distance metric, and the option travels all the way to the endpoint of the recursion where, if present, it governs the metric actually used.
- There is a setting that determines whether the distance metric reports its method if the setting is true. Like “ReportMetricMethod” as a boolean with default value of false.
- Another option would be that the result of the evaluation of the distance metric is a tuple, the first element of which is the distance, and the second element of which is the method.
PS. I had no idea that computing MNrmTrg and MNrmStE were as expensive as you say — consuming 70 percent of the compute time for each iteration (really? did I get that right?). Maybe another alternative would be to set things up so that there could be a crude metric (say, the default) and a refined metric that gets computed optionally, and only once the crude metric is satisfied.
In any case, for reasons I will explain when your mini-family returns from its vacation, this is actually a high priority at the moment. (cc’ing Decory because it’s high priority derives from things he is working on).
On Tue, Aug 8, 2023 at 10:07 AM Alan Lujan @. @.> wrote:
I think one compromise is that we can define the class method in-place to
overwrite the default recursive look up.
class ConsumerSolution(MetricObject):
.... def distance(self, other): return custom_distance
This should overwrite the default case when implemented.
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669691494, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAKCK7ZLKBWHFJDMQZO73A3XUJB2JANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: <econ-ark/HARK/issues/1331/1669691494 @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669870221, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFJHCG3EOSRIULOCICDXUJNTNANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: @.***>
When solving using EGM, if we're not going to use the Trg or StE but instead the max Euclidian distance, it seems like the natural thing to look at would be the consumed function (that is, c as a function of a) since the aGrid is identical from iteration to iteration.
On Tue, Aug 8, 2023 at 12:35 PM Matthew N. White @.***> wrote:
You're correct, under the default parameters, 70% of computation time is spent on calc_stable_points. If you crank up the number of nodes in the income distribution (so that integration is very expensive), that balance changes.
It costs a lot because it's a sequential or iterative search, one value at a time, as it zooms in to find the targets.
On Tue, Aug 8, 2023, 11:48 AM Christopher Llorracc Carroll < @.***> wrote:
I’d be quite satisfied with a fix in which:
- The user can pass an option to the distance metric, and the option travels all the way to the endpoint of the recursion where, if present, it governs the metric actually used.
- There is a setting that determines whether the distance metric reports its method if the setting is true. Like “ReportMetricMethod” as a boolean with default value of false.
- Another option would be that the result of the evaluation of the distance metric is a tuple, the first element of which is the distance, and the second element of which is the method.
PS. I had no idea that computing MNrmTrg and MNrmStE were as expensive as you say — consuming 70 percent of the compute time for each iteration (really? did I get that right?). Maybe another alternative would be to set things up so that there could be a crude metric (say, the default) and a refined metric that gets computed optionally, and only once the crude metric is satisfied.
In any case, for reasons I will explain when your mini-family returns from its vacation, this is actually a high priority at the moment. (cc’ing Decory because it’s high priority derives from things he is working on).
On Tue, Aug 8, 2023 at 10:07 AM Alan Lujan @. @.> wrote:
I think one compromise is that we can define the class method in-place to
overwrite the default recursive look up.
class ConsumerSolution(MetricObject):
.... def distance(self, other): return custom_distance
This should overwrite the default case when implemented.
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669691494,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AAKCK7ZLKBWHFJDMQZO73A3XUJB2JANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: <econ-ark/HARK/issues/1331/1669691494 @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669870221, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ADKRAFJHCG3EOSRIULOCICDXUJNTNANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669953839, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKCK7ZQIX2JVVQUMEQ4WADXUJTGZANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: @.***>
--
The "consumed" function doesn't currently get written to the solution object, but that can be changed. But we would still have to update our classes so that the distance metric attribute can be changed at the instance level, rather than innate to the class. The class still needs to come with a default for instantiation, but can be overwritten on an instance-by-instance basis.
On Tue, Aug 8, 2023, 4:34 PM Christopher Llorracc Carroll < @.***> wrote:
When solving using EGM, if we're not going to use the Trg or StE but instead the max Euclidian distance, it seems like the natural thing to look at would be the consumed function (that is, c as a function of a) since the aGrid is identical from iteration to iteration.
On Tue, Aug 8, 2023 at 12:35 PM Matthew N. White @.***> wrote:
You're correct, under the default parameters, 70% of computation time is spent on calc_stable_points. If you crank up the number of nodes in the income distribution (so that integration is very expensive), that balance changes.
It costs a lot because it's a sequential or iterative search, one value at a time, as it zooms in to find the targets.
On Tue, Aug 8, 2023, 11:48 AM Christopher Llorracc Carroll < @.***> wrote:
I’d be quite satisfied with a fix in which:
- The user can pass an option to the distance metric, and the option travels all the way to the endpoint of the recursion where, if present, it governs the metric actually used.
- There is a setting that determines whether the distance metric reports its method if the setting is true. Like “ReportMetricMethod” as a boolean with default value of false.
- Another option would be that the result of the evaluation of the distance metric is a tuple, the first element of which is the distance, and the second element of which is the method.
PS. I had no idea that computing MNrmTrg and MNrmStE were as expensive as you say — consuming 70 percent of the compute time for each iteration (really? did I get that right?). Maybe another alternative would be to set things up so that there could be a crude metric (say, the default) and a refined metric that gets computed optionally, and only once the crude metric is satisfied.
In any case, for reasons I will explain when your mini-family returns from its vacation, this is actually a high priority at the moment. (cc’ing Decory because it’s high priority derives from things he is working on).
On Tue, Aug 8, 2023 at 10:07 AM Alan Lujan @. @.> wrote:
I think one compromise is that we can define the class method in-place to
overwrite the default recursive look up.
class ConsumerSolution(MetricObject):
.... def distance(self, other): return custom_distance
This should overwrite the default case when implemented.
— Reply to this email directly, view it on GitHub < https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669691494>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AAKCK7ZLKBWHFJDMQZO73A3XUJB2JANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: <econ-ark/HARK/issues/1331/1669691494 @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669870221,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ADKRAFJHCG3EOSRIULOCICDXUJNTNANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669953839, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AAKCK7ZQIX2JVVQUMEQ4WADXUJTGZANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1670270011, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKRAFKY7AFQSQSE7PLNOXDXUKPHFANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: @.***>
OK, that seems like the right thing to do.
On Tue, Aug 8, 2023 at 4:51 PM Matthew N. White @.***> wrote:
The "consumed" function doesn't currently get written to the solution object, but that can be changed. But we would still have to update our classes so that the distance metric attribute can be changed at the instance level, rather than innate to the class. The class still needs to come with a default for instantiation, but can be overwritten on an instance-by-instance basis.
On Tue, Aug 8, 2023, 4:34 PM Christopher Llorracc Carroll < @.***> wrote:
When solving using EGM, if we're not going to use the Trg or StE but instead the max Euclidian distance, it seems like the natural thing to look at would be the consumed function (that is, c as a function of a) since the aGrid is identical from iteration to iteration.
On Tue, Aug 8, 2023 at 12:35 PM Matthew N. White @.***> wrote:
You're correct, under the default parameters, 70% of computation time is spent on calc_stable_points. If you crank up the number of nodes in the income distribution (so that integration is very expensive), that balance changes.
It costs a lot because it's a sequential or iterative search, one value at a time, as it zooms in to find the targets.
On Tue, Aug 8, 2023, 11:48 AM Christopher Llorracc Carroll < @.***> wrote:
I’d be quite satisfied with a fix in which:
- The user can pass an option to the distance metric, and the option travels all the way to the endpoint of the recursion where, if present, it governs the metric actually used.
- There is a setting that determines whether the distance metric reports its method if the setting is true. Like “ReportMetricMethod” as a boolean with default value of false.
- Another option would be that the result of the evaluation of the distance metric is a tuple, the first element of which is the distance, and the second element of which is the method.
PS. I had no idea that computing MNrmTrg and MNrmStE were as expensive as you say — consuming 70 percent of the compute time for each iteration (really? did I get that right?). Maybe another alternative would be to set things up so that there could be a crude metric (say, the default) and a refined metric that gets computed optionally, and only once the crude metric is satisfied.
In any case, for reasons I will explain when your mini-family returns from its vacation, this is actually a high priority at the moment. (cc’ing Decory because it’s high priority derives from things he is working on).
On Tue, Aug 8, 2023 at 10:07 AM Alan Lujan @. @.> wrote:
I think one compromise is that we can define the class method in-place to
overwrite the default recursive look up.
class ConsumerSolution(MetricObject):
.... def distance(self, other): return custom_distance
This should overwrite the default case when implemented.
— Reply to this email directly, view it on GitHub < https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669691494>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AAKCK7ZLKBWHFJDMQZO73A3XUJB2JANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: <econ-ark/HARK/issues/1331/1669691494 @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub < https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669870221>,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/ADKRAFJHCG3EOSRIULOCICDXUJNTNANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1669953839,
or unsubscribe <
https://github.com/notifications/unsubscribe-auth/AAKCK7ZQIX2JVVQUMEQ4WADXUJTGZANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
--
- Chris Carroll
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1670270011, or unsubscribe < https://github.com/notifications/unsubscribe-auth/ADKRAFKY7AFQSQSE7PLNOXDXUKPHFANCNFSM6AAAAAA3EVWDLI>
. You are receiving this because you were mentioned.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/econ-ark/HARK/issues/1331#issuecomment-1670291233, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKCK7Y2BTNEFKD3YJSUFEDXUKRERANCNFSM6AAAAAA3EVWDLI . You are receiving this because you were mentioned.Message ID: @.***>
--
We need to make it much more transparent how we are comparing the distances between our solutions and solution objects. Currently the way to do it is very labyrinthian and to figure it out you have to know how to follow a long rabbit hole.
We should be able to specify clearly what the distance criteria is and the user should be able to understand how that is happening readily.
@llorracc