search

econ-ark / EstimatingMicroDSOPs

https://econ-ark.github.io/EstimatingMicroDSOPs/
MIT License
2 stars 10 forks source link

Drop people with income == 0! #5

Closed Mv77 closed 2 years ago

Mv77 commented 2 years ago

The current code uses SCF data to find (and then target in estimation) median wealth-to-income ratios. But, it does not drop people with income == 0, so some target moments are nan and the objective function is nan for every set of parameters... at least for me.

Does anyone else run into the same issue?

This PR fixes the issue.

llorracc commented 2 years ago

Puzzled why we didn't find this earlier. Is it ready to merge? Or WIP? If ready to merge, please add that tag, and assign someone (me; Tao; Alan) to review and merge.

On Tue, Jul 19, 2022 at 9:40 AM Mateo Velásquez-Giraldo < @.***> wrote:

The current code uses SCF data to find (and then target in estimation) median wealth-to-income ratios. But, it does not drop people with income == 0, so some target moments are nan and the objective function is nan for every set of parameters... at least for me.

Does anyone else run into the same issue?

This PR fixes the issue.

You can view, comment on, or merge this pull request online at:

https://github.com/econ-ark/SolvingMicroDSOPs/pull/5 Commit Summary

File Changes

(1 file https://github.com/econ-ark/SolvingMicroDSOPs/pull/5/files)

Patch Links:

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

--

Mv77 commented 2 years ago

It is ready, but I don't think I have permissions to add tags or reviewers.

@llorracc @alanlujan91 could any of you confirm that the current value function is NaN? Just run the structural estimation script with a debug on the line that returns the loss function. If you get NaN, this is ready to merge and fixes it.

alanlujan91 commented 2 years ago

Can't tag or review either.

I'm not familiar with the structural estimation part of smDSOPs, do you mean to debug the following line?

https://github.com/econ-ark/SolvingMicroDSOPs/blob/ed6f6f4ce4d8ed36ca1400427f0afca193e1ac21/Code/StructEstimation.py#L299

Mv77 commented 2 years ago

This line: https://github.com/econ-ark/SolvingMicroDSOPs/blob/ed6f6f4ce4d8ed36ca1400427f0afca193e1ac21/Code/StructEstimation.py#L193

Just check if distance_sum is nan before it returns.

llorracc commented 2 years ago

Oh, yes, the tags I was talking about are in HARK but not SolvingMicroDSOPs. But I've added both of you as collaborators, so I think that should let you create a tag.

On Thu, Jul 21, 2022 at 11:05 AM Mateo Velásquez-Giraldo < @.***> wrote:

It is ready, but I don't think I have permissions to add tags or reviewers.

@llorracc https://github.com/llorracc @alanlujan91 https://github.com/alanlujan91 could any of you confirm that the current value function is NaN? Just run the structural estimation script with a debug on the line that returns the loss function. If you get NaN, this is ready to merge and fixes it.

— Reply to this email directly, view it on GitHub https://github.com/econ-ark/SolvingMicroDSOPs/pull/5#issuecomment-1191598964, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKCK754FRALDIP5KBYUTITVVFRMRANCNFSM54AA2KGA . You are receiving this because you were mentioned.Message ID: @.***>

--

alanlujan91 commented 2 years ago

OK, distance_sum is nan in master, but PR fixes it!