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.

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

Knockoffs(1/4): add comments and docstring of the functions #128

Open
wants to merge 47 commits into
base: main
Choose a base branch
from

Conversation

lionelkusch
Copy link
Collaborator

I aggregated most of the file for knockoff in one file.
I separate the main method from the computation of p_value and e_value and associate tests.

@lionelkusch lionelkusch requested review from jpaillard and removed request for jpaillard January 15, 2025 10:40
@lionelkusch lionelkusch requested a review from bthirion January 15, 2025 10:41
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 99.66997% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.88%. Comparing base (19e6cf7) to head (4e3f4d2).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/hidimstat/utils.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   81.70%   82.88%   +1.17%     
==========================================
  Files          43       39       -4     
  Lines        2312     2366      +54     
==========================================
+ Hits         1889     1961      +72     
+ Misses        423      405      -18     

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

@lionelkusch
Copy link
Collaborator Author

lionelkusch commented Jan 15, 2025

Some modifications of the code were done following the issue #125.

@lionelkusch lionelkusch requested review from jpaillard and AngelReyero and removed request for bthirion January 15, 2025 16:36
model_x_knockoff_filter
model_x_knockoff_pvalue
model_x_knockoff_bootstrap_quantile
model_x_knockoff_bootstrap_e_value
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these functions meant to be public ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they should be public.

src/hidimstat/gaussian_knockoff.py Outdated Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Outdated Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Outdated Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Outdated Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Outdated Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Outdated Show resolved Hide resolved
src/hidimstat/gaussian_knockoff.py Show resolved Hide resolved
@lionelkusch lionelkusch removed the request for review from jpaillard January 17, 2025 13:25
@lionelkusch
Copy link
Collaborator Author

I merge the function model_x_knockoff and model_x_knockoff_aggregation together for having a unique function.

@bthirion
Copy link
Contributor

LMK when you want another review

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.

2 participants