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