-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: master
Are you sure you want to change the base?
Conversation
@sahahn Hi Sage, I'll review your pull request shortly and will write a review here afterwards -- thanks for contributing to the project! |
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? |
Hi fabioboh, sorry, but unfortunately not for me. I've moved on from academia and don't have any free time right now :( |
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 :-) |
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:
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.
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.
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.
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