Skip to content

Conversation

jpaillard
Copy link
Collaborator

@jpaillard jpaillard commented Sep 2, 2025

  • replace argument dicts by sklearn estimators
  • remove the function _fit_lasso, replaced by cloned models
  • Create a separate function lasso_screen for clarity
  • add tests to see if the method works (identify truly important features) in a linear setting

Copy link

codecov bot commented Sep 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.08%. Comparing base (57d40de) to head (32b7b8d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #370      +/-   ##
==========================================
- Coverage   98.10%   98.08%   -0.02%     
==========================================
  Files          22       22              
  Lines        1159     1148      -11     
==========================================
- Hits         1137     1126      -11     
  Misses         22       22              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jpaillard jpaillard marked this pull request as ready for review September 2, 2025 13:00
@lionelkusch
Copy link
Collaborator

I think it's better to remove the screening procedure from D0CRT and add the screening function and the fit_function in another file.
What do you think?

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.

Thx. I think its' an improvement. Wondering if we can further simplify.

@jpaillard
Copy link
Collaborator Author

Would it actually make sense to have a Screening class instead of an independent function?
The class could not only be compatible with Lasso but also the univariate methods from #317

@lionelkusch
Copy link
Collaborator

Would it actually make sense to have a Screening class instead of an independent function? The class could not only be compatible with Lasso but also the univariate methods from #317

This can provide confusion between selection method and the Screening class.
We can add a marginal class using Lasso to determine the marginal features importance and use it to replace the actual screening process. In this case, any marginal method can be used before the importance of another method for making a preselection of features.

@jpaillard jpaillard requested a review from bthirion September 4, 2025 06:49
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.

Almost ready to merge, thx !


# to explain the data. While d0CRT can use any estimator, its distillation step
# restricts it from capturing variable interactions. A more advanced distillation
# approach, such as d1CRT, may help overcome this limitation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we provide no implementation of d1CRT, we should not make any claim on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the last sentence

Copy link
Collaborator

@lionelkusch lionelkusch left a comment

Choose a reason for hiding this comment

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

Propose you small modification for improving the code.

jpaillard and others added 5 commits September 11, 2025 15:26
…f-parameter-dictionaries' of github.com:mind-inria/hidimstat into 369-d0crt-accept-scikit-learn-models-directly-instead-of-parameter-dictionaries
Copy link
Collaborator

@lionelkusch lionelkusch left a comment

Choose a reason for hiding this comment

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

thank you for the change.

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.

LGTM, thx.

@jpaillard jpaillard merged commit 379fce6 into main Sep 12, 2025
20 checks passed
@jpaillard jpaillard deleted the 369-d0crt-accept-scikit-learn-models-directly-instead-of-parameter-dictionaries branch September 12, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants