search

JMSLab / eventstudyr

Other
24 stars 1 forks source link

Address Static Special Case #5

Closed nateschor closed 2 years ago

nateschor commented 2 years ago

In this issue we'll modify the EventStudy() function to handle the static special case of M = G = LM = LG = 0 from equation (2) in EventStudy

Next steps (can be modified):

jmshapir commented 2 years ago

Following our discussions in https://github.com/JMSLab/EventStudy/issues/206, I think our reasoning in https://github.com/JMSLab/eventstudyr/pull/3#discussion_r825372237 was off because we were summing over indices "in the wrong direction."

@veli-m-andirin @nateschor as a first step here, I think it could be a good idea to confirm that the code is only ever executing the summation in equation (2) from a lower to a higher index. If so then I think the static model should already be implemented as a special case of the dynamic model in equation (2).

If not we may want to repair!

Thanks again @veli-m-andirin @nateschor for bringing up these points.

veli-m-andirin commented 2 years ago

Thanks @jmshapir! During our call today we went over the script with @nateschor and noticed that unfortunately we don't distinguish the cases where the lower index is larger than the higher index. We'll indeed need to fix this, and ignore the summation for such cases.

jmshapir commented 2 years ago

Thanks @veli-m-andirin! I assume we'll repair it as part of this issue #5 but let me know if you have a different plan in mind.

veli-m-andirin commented 2 years ago

Thanks @jmshapir! Exactly, this is the plan for this issue.

veli-m-andirin commented 2 years ago

Created the branch issue5_static_case to work on this issue.

veli-m-andirin commented 2 years ago

@nateschor, in addition to the case with G=M=L_G=L_M=0, I tried to see how our code performs in general when M + L_M -1 <= -(G + L_G), because that means we have either zero or one item in the summation given in the equation (2) of the paper.

My explorations there led me to see that our code in general performs poorly when either one of M+L_M-1<1 or G+L_G<1 holds. Here is a summary of what I worked on. At the end of the document, I made a general summary of the issues and proposed solutions. I think it would be great if you could check the reasoning in the document. I plan to implement the architecture I proposed in this document.

@jmshapir, fyi. In case you'd like to have a look at the document I posted but find it too long (I had to write down several cases in detail to get a better sense of the problems), the sections "Summary of the problems" and "Solutions" might be more efficient to read.

nateschor commented 2 years ago

@veli-m-andirin this is great, thank you! The only comment I had was in bd51dbc about whether "shouldn" meant "should" or "should not".

After that, implementing your proposed architecture sounds good to me. Please let me know if you would like help with implementation!

veli-m-andirin commented 2 years ago

Continues in PR #6.

veli-m-andirin commented 2 years ago

Summary: In this issue we fixed a couple instances where our code produced wrong results. One of those is the static special case where the event-study window parameters in equation (2) of the paper (namely M, G, L_G, and L_M) are all equal to zero. For an exploration of such cases see the document here.

Changes were merged into master in a697882.

Final state of the issue branch is here.

Final state of the issue sub is here.