Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Identity Weighting Matrix #477

Merged
merged 3 commits into from
Feb 10, 2024
Merged

Identity Weighting Matrix #477

merged 3 commits into from
Feb 10, 2024

Conversation

sidd3888
Copy link
Collaborator

@sidd3888 sidd3888 commented Feb 7, 2024

Added the identity weighting matrix to the MSM estimation:

  • First change made is to the get_weighting_matrix() function to allow for the identity matrix
  • Second change permits "identity" as a valid option for weights in estimate_msm()

@sidd3888 sidd3888 requested a review from janosg February 7, 2024 09:36
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b77deb8) 92.98% compared to head (2fef810) 92.98%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #477   +/-   ##
=======================================
  Coverage   92.98%   92.98%           
=======================================
  Files         193      193           
  Lines       14631    14635    +4     
=======================================
+ Hits        13605    13609    +4     
  Misses       1026     1026           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@janosg
Copy link
Member

janosg commented Feb 9, 2024

Hi @sidd3888, thank you very much for your Pull Request. The changes are great. There is just one more thing: It would be great to also have a small test case for the get_weighting_matrix with the "identity" option.

Let me know whether you want to add this test or I should do it. If you want to add the test, it should be in this file

@sidd3888
Copy link
Collaborator Author

sidd3888 commented Feb 9, 2024

If I understand the code there correctly, this would require nothing more than just adding "identity" as a possible option (and accordingly altering the test) for the get_weighting_matrix test right? I can add that to this PR right away.

@janosg
Copy link
Member

janosg commented Feb 9, 2024

Yes. Thank you so much!

@janosg janosg merged commit 46b024f into main Feb 10, 2024
15 checks passed
@janosg janosg deleted the identity_weights branch February 10, 2024 08:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants