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

[MAINT] Update pinned versions #15

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

Conversation

chiuhoward
Copy link

Put changes on a separate branch.

  1. Vendorized neurocombat_sklearn and changed imports to fix OneHotEncoder sparse_output argument (Incompatible with scikit-learn >= 1.2 Warvito/neurocombat_sklearn#3).
  2. Modified estimator to replace deprecated base_estimator.

@arokem
Copy link
Contributor

arokem commented Dec 14, 2024

Hey @chiuhoward : this is a great start. I just resolved a small conflict with main and the CI is set in motion.

I suggest that rather than cloning neurocombat_sklearn as a submodule (I still don't understand how those work...), we should copy that code (together with it's tests and license) into a module of our repository and fix it here. That way, we can keep maintaining that code into the future here (and maybe eventually upstream it to nilearn?)

@arokem
Copy link
Contributor

arokem commented Dec 14, 2024

Also: I see that the CI is failing because of linting errors. Make sure to install the pre-commit hooks of this repository into your development environment, by running the following in that env and in the top level folder of this repo:

pip install pre-commit 
pre-commit install

That way, every time you make a commit, the linter (ruff) will run on the repo and either fix linting issues or tell you how to fix these issues.

@chiuhoward
Copy link
Author

@arokem I think I need more help at this point - something about numpy objects and estimators not having a fit method. Also I've started to rename stuff in the tests too, but I'm not sure if that's good practice either - I could just be breaking good tests so that they pass on my bad code?

@arokem
Copy link
Contributor

arokem commented Dec 20, 2024

Hi @chiuhoward : I think that it would be good to take a divide and conquer approach here, focusing on one test at a time. I would focus first on making the tests that came in together with the neurocombat_sklearn code pass. Once you know that you've updated that code to support newer versions of sklearn, you can move on to update our code to match that.

@arokem
Copy link
Contributor

arokem commented Dec 21, 2024

One more small thing: I would remove the neurocombat_sklearn folder that you have at the top level and move the tests for neurocombat_sklearn to be together with all the other tests.

Actually: instead of explaining, I'll make a PR with that change against your branch. Stand by.

@arokem
Copy link
Contributor

arokem commented Dec 21, 2024

OK - when you merge this PR: chiuhoward#2, that should get into this branch (and you'll need to pull after you do that, so you get that updated locally as well).

Removes an extraneous (?) copy of neurocombat sklearn
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