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

Add alternate class based API #15

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

sahahn
Copy link

@sahahn sahahn commented May 11, 2022

Hi,
Thanks for maintaining the python version of Combat. I just finished writing an alternate API for the code based on a scikit-learn style API. If you all are open to it, I thought it could be a nice addition to the code already maintained here as a complementary option.

This pull request contains a few different things:

  1. The biggest is the addition of the new Combat class, e.g., from neuroCombat import Combat, which makes a few different choices with respect to formatting input arguments, but most of the actual logic and implementation of combat is the same as the code you have (but re-written in the scikit-learn fit, transform, fit_transform) style.

  2. With respect to the Combat class, the only area where results really differ is with respect to the neuroCombatFromTraining, or in applying combat to unseen data. Basically, I noticed that for now the current method doesn't accept covariates and instead just uses the mean calculated mod_mean from the training set. So what I did, was for the new default set it to instead compute the actual values based on the new samples co-variate values. Then, there is an optional flag for including y, or the target variable in the sci-kit learn setup, which if you want to try and preserve the value of y while running combat, will essentially add it as another co-variate, but when predicting new samples will do something similar to the already implemented method and just revert to using the mean value. I can discuss these changes more if needed, to make sure I'm thinking about it the right way.

  3. I wrote a number of tests, mostly confirming base behavior and ensuring that expected output matches between my version and the existing version of the code. Further, I saw you had a travis CI yml file, but no test code that I could find, so I setup a new CI pipeline through github actions to run the tests. There are definitely more tests that can and should be added.

  4. I made some small changes to the setup.py and README, ect... mostly to reflect the new changes and provide some new examples

Thanks,
Sage

@Jfortin1
Copy link
Owner

@sahahn Hi Sage, I'll review your pull request shortly and will write a review here afterwards -- thanks for contributing to the project!

@fabioboh
Copy link

dear @sahahn and @Jfortin1 thank you very much for providing/maintaining and constantly improving a python version of combat. personally I find the contribution by @sahahn very useful. actually @sahahn , would it be possible to provide the same extension, i.e. the addition of the new Combat class in the scikit-learn style also for the https://github.com/rpomponio/neuroHarmonize implementation of Combat?

@sahahn
Copy link
Author

sahahn commented Feb 15, 2023

Hi fabioboh, sorry, but unfortunately not for me. I've moved on from academia and don't have any free time right now :(

@fabioboh
Copy link

there is actually no need for the same extension on the neuroHarmonize package, it is enough to write some kind of class CombatGAMTransform(BaseEstimator, TransformerMixin): with fit and transform methods :-)

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.

3 participants