Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

fix: the way essential covariates and one-hot-encoded ones are processed #256

Merged
merged 5 commits into from
Jan 21, 2025
Merged

fix: the way essential covariates and one-hot-encoded ones are processed #256

merged 5 commits into from
Jan 21, 2025

Conversation

MarIniOnz
Copy link
Contributor

  • Essential covariates are no longer "gender" and "age" by default, but all attributes from the patients group. Raises errors in case there are not enough control/treated patients with those attributes.

  • One-hot-encoded variables are not "gender" by default, but all attributes characterised by extract_attribute_summary function as Categorical.

  • Added tests to the Matching Class

  • Changed some variable names (hyperparam -> hyperparameters) for better variable naming consistency.

@MarIniOnz MarIniOnz requested review from JabobKrauskopf and a team as code owners November 19, 2024 10:44
@MarIniOnz MarIniOnz linked an issue Nov 19, 2024 that may be closed by this pull request
Default values for essential_covariates in TreatmentEffect #254
Closed
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from 00b086d to 1fe006e Compare December 2, 2024 13:47
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from 1fe006e to 866207a Compare December 17, 2024 10:52
LauraBoenchenLB
LauraBoenchenLB requested changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Looks great! A few changes needed, but overall very well executed!

medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Show resolved Hide resolved
medmodels/treatment_effect/matching/tests/test_matching.py Show resolved Hide resolved
medmodels/treatment_effect/matching/tests/test_matching.py Outdated Show resolved Hide resolved
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from d169a70 to e41a9ff Compare December 17, 2024 17:06
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch 2 times, most recently from d3469d3 to 6f0eba2 Compare January 7, 2025 09:34
LauraBoenchenLB
LauraBoenchenLB previously approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Looks great! All my concerns have been addressed, very happy with the result! 🦭

@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from 6f0eba2 to 7adcc78 Compare January 14, 2025 10:33
LauraBoenchenLB
LauraBoenchenLB previously approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Let's meeeerge 🦭

JabobKrauskopf
JabobKrauskopf requested changes Jan 14, 2025
View reviewed changes
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/matching.py Outdated Show resolved Hide resolved
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from d319202 to f1d99ae Compare January 15, 2025 12:51
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from f1d99ae to ca753ba Compare January 15, 2025 16:35
LauraBoenchenLB
LauraBoenchenLB requested changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Minor detail change request!

medmodels/treatment_effect/matching/neighbors.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/propensity.py Outdated Show resolved Hide resolved
LauraBoenchenLB
LauraBoenchenLB previously approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Perfect! This get's an approval 🤩

@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch 2 times, most recently from 4bfb580 to 39ad1cd Compare January 20, 2025 15:39
@MarIniOnz MarIniOnz force-pushed the 254-default-values-for-essential_covariates-in-treatmenteffect branch from 39ad1cd to 9ef8d28 Compare January 21, 2025 09:46
JabobKrauskopf
JabobKrauskopf requested changes Jan 21, 2025
View reviewed changes
Copy link
Member

@JabobKrauskopf JabobKrauskopf left a comment

Choose a reason for hiding this comment

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

smol stuff

medmodels/treatment_effect/matching/neighbors.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/treatment_effect.py Outdated Show resolved Hide resolved
medmodels/treatment_effect/matching/propensity.py Outdated Show resolved Hide resolved
JabobKrauskopf
JabobKrauskopf approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@JabobKrauskopf JabobKrauskopf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I told ChatGPT to surprise me. This is what it came up with. I'm scared.

The seals won the Seal-Human war.

Screen Shot 2025-01-21 at 11 45 19

@MarIniOnz
Copy link
Contributor Author

I told ChatGPT to surprise me. This is what it came up with. I'm scared.

The seals won the Seal-Human war.

Screen Shot 2025-01-21 at 11 45 19

that is the future.

JabobKrauskopf
JabobKrauskopf approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@JabobKrauskopf JabobKrauskopf left a comment

Choose a reason for hiding this comment

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

This is apparently the continuation of the story

Screen Shot 2025-01-21 at 11 58 34

LauraBoenchenLB
LauraBoenchenLB reviewed Jan 21, 2025
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Tiny question, will approve after! <3

medmodels/treatment_effect/matching/matching.py Show resolved Hide resolved
LauraBoenchenLB
LauraBoenchenLB approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@LauraBoenchenLB LauraBoenchenLB left a comment

Choose a reason for hiding this comment

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

Approooove 🦭

@JabobKrauskopf JabobKrauskopf merged commit 2e4ee97 into main Jan 21, 2025
6 checks passed
@JabobKrauskopf JabobKrauskopf deleted the 254-default-values-for-essential_covariates-in-treatmenteffect branch January 21, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LauraBoenchenLB LauraBoenchenLB LauraBoenchenLB approved these changes

@OFranke OFranke Awaiting requested review from OFranke OFranke is a code owner automatically assigned from limebit/medmodels-software-engineering

@FloLimebit FloLimebit Awaiting requested review from FloLimebit FloLimebit is a code owner automatically assigned from limebit/medmodels-software-engineering

@philippgrosser philippgrosser Awaiting requested review from philippgrosser philippgrosser is a code owner automatically assigned from limebit/medmodels-data-science-team

@JabobKrauskopf JabobKrauskopf Awaiting requested review from JabobKrauskopf JabobKrauskopf is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default values for essential_covariates in TreatmentEffect
3 participants
@MarIniOnz @JabobKrauskopf @LauraBoenchenLB