Skip to content

EnCluDL(1/4): add comments and docstring of the functions #156

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
Open
wants to merge 132 commits into
base: main
Choose a base branch
from
Open

EnCluDL(1/4): add comments and docstring of the functions #156

wants to merge 132 commits into from

Conversation

lionelkusch
Copy link
Collaborator

I refactor the code of ensemble_clustered_inference and clustered_inference.
This pull request is dependent of the PR #127

@lionelkusch
Remove the method parameter because only lasso is supported
99fa174
@lionelkusch
Comment reid procedure
3eebe97
@lionelkusch
update comment
ffbb6e9
@lionelkusch
Comments desparsified and reorganise the structure
f92ea4d
@lionelkusch
Improve comment and let's 2 question?
d465c36
@lionelkusch
Comment deparsified lasso grouping
eeae34b
@lionelkusch
format
d23e3d6
@lionelkusch
comment group_red
4ad785c
@lionelkusch
Comment emperical snr
4787c27
@lionelkusch
Comment the side function of desparsified lasso
97ff04f
@lionelkusch
Fix bugs
4cdaf90
@lionelkusch
Put back stationnary noise
931e52f
@lionelkusch
Fix the changement of signature
d3d91e8
@lionelkusch
Modify documentation to include new function
92c7f57
@lionelkusch
Add a question
8366e70
@lionelkusch
Improve commit
e81e3db
@lionelkusch
remove unecesary parameter
9052326
@lionelkusch
Merge branch 'main' into PR_desparsified_lasso
7e4435a
@lionelkusch
Fix citation
8068e89
@lionelkusch
Add reference to citation
b4055a3
@lionelkusch
Improve docstring with copilot
109dcd2
@lionelkusch
Add test
0ca0ec1
@lionelkusch
Improve docstring
977cd30
@lionelkusch
Format files
f25cc8c
@lionelkusch
Format files
f304a5e
@lionelkusch
Remove memory parameters
aa44640
@lionelkusch
Update the function
ab0ebd5
@lionelkusch
Group the function in one
9081811
@lionelkusch
Update the tests
e2dd939
@lionelkusch
Fix usage of the function
aea49d6
bthirion
bthirion reviewed Apr 5, 2025
View reviewed changes
Copy link
Collaborator

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

The example on fMRI data does not render well, for some reason. Can you check that, so that we can merge this PR and proceed to further improvements ?

@lionelkusch
Copy link
Collaborator Author

The example on fMRI data does not render well, for some reason. Can you check that, so that we can merge this PR and proceed to further improvements ?

What do you mean that fMRI does not render well?
For me there are very similar to the actual ones.

@bthirion
Copy link
Collaborator

The EncluDL image is ugly with the white area. Something went wrong.

@lionelkusch
Copy link
Collaborator Author

I added an empirical threshold for filling the gap.

@lionelkusch lionelkusch added the API 1 Refactoring following API 1 label Apr 11, 2025
@bthirion
Copy link
Collaborator

We still have the issue in the example ?
https://output.circle-artifacts.com/output/job/fec94289-10d3-4ac9-aac5-e256ede62dde/artifacts/0/doc_conf/auto_examples/plot_fmri_data_example.html#sphx-glr-auto-examples-plot-fmri-data-example-py

@lionelkusch
Copy link
Collaborator Author

We still have the issue in the example ? https://output.circle-artifacts.com/output/job/fec94289-10d3-4ac9-aac5-e256ede62dde/artifacts/0/doc_conf/auto_examples/plot_fmri_data_example.html#sphx-glr-auto-examples-plot-fmri-data-example-py

I am sorry, I was mixing the example 2D and the example fMRI.

@lionelkusch
Copy link
Collaborator Author

The white colour came from the modification of the aggregation function.
I realise that this aggregation is similar to that for the knockoff. I updated the aggregation method of EnCluDL based on the knockoff.
Can @bthirion or @AngelReyero validate my modification of the function ensemble_clustered_inference_pvalue (line 496 of ensemble_clustered_inference.py)?

bthirion
bthirion approved these changes Apr 14, 2025
View reviewed changes
Copy link
Collaborator

@bthirion bthirion left a comment

Choose a reason for hiding this comment

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

The example renders well, thx !

examples/plot_fmri_data_example.py
X, y, ward, n_clusters, scaler_sampling=StandardScaler()
)
beta_hat, pval_cdl, _, one_minus_pval_cdl, _ = clustered_inference_pvalue(
X.shape[0], None, ward_, beta_hat, theta_hat, omega_diag
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll have to refactor this to a class (same logic as discussed for DCRT etc.)
Are you planing to do that in a forthcoming PR?

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, I plan to refactor all of this in classes in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bthirion bthirion bthirion approved these changes

@jpaillard jpaillard jpaillard approved these changes

Assignees
No one assigned
Labels
API 1 Refactoring following API 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lionelkusch @jpaillard @bthirion @actions-user