Skip to content

Mnar and directopt#61

Draft
kyuseongchoi5 wants to merge 14 commits intomainfrom
mnar_and_directopt
Draft

Mnar and directopt#61
kyuseongchoi5 wants to merge 14 commits intomainfrom
mnar_and_directopt

Conversation

@kyuseongchoi5
Copy link
Collaborator

Added mnar option in loader.py
Added fit_method.py that implements direct optimization
Added mcar distributional data generator in the mcar.py file

Copy link
Collaborator

@albertgong1 albertgong1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall (see inline comments)

def _make_mnar(self) -> None:
raise NotImplementedError("MNAR yet to be implemented")
# TODO: Aashish/Caleb/Kyuesong/Tatha: come back to this later
"""Makes values missing not at random (MNAR), specifically staggered adoption (non-positivity & confounded)"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what each of these terms mean in the doctoring:

  1. non-positivity
  2. confounded

g3_inds = np.arange(2 * N // 3, N)

gamma_1 = [2, 0.7, 1, 0.7]
gamma_2 = [2, 0.2, 1, 0.2]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the significance of γ1, γ2? These appear as magic numbers. Is there a way we can parametrize this? Should also put some documentation (inline or at the top) to explain how this entire function works.

1,
self._expit(
gamma_1[0]
+ (0.99**t) * gamma_1[1] * U[i - 1]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is 0.99 hardcoded inline? Could this be assigned a constant value near the beta? Ideally this is a parameter too

1,
self._expit(
gamma_2[0]
+ (1.01**t) * gamma_2[1] * U[i - 1]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same issue as 0.99. Maybe it should be 2 - new_const_name? What is this for anyway?

# Data[Masking == 0] = Y0[Masking == 0]
Data: np.ndarray = np.array(Y)
return Data, Theta, Masking

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this entire file be deleted? Why are we adding code here? Ideally all mcar and mnar code sits inside the same class, with different parameters that may affect how we wish to do MCAR/MNAR

@albertgong1 albertgong1 linked an issue Mar 19, 2025 that may be closed by this pull request
@aashish-khub aashish-khub marked this pull request as draft April 29, 2025 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement direct optimization tuning method Upload Kyuseong's data generation and mask generation code for distributional setting

3 participants