Closed jrasero closed 3 years ago
Pradeep, I was not able to make tox tests pass. It seems to be giving me errors because it does not find python3.4 and python3.5.
The pull-request passed both flake8 and the tests locally, with python3.8.
Wonderful! Thanks Javier, this is a fantastic PR I've been looking forward to! :) Let me take a look and make detailed comments. PS: I am currently travelling in India with limited / intermittent internet capabilities, so I might be slower than normal to respond.
Hi Javi, sorry for the delay in my reply. I am going through this now, and feel like it helps to have a simple example on how to use this class/method, and as well as some pseudocode on what results we should expect when we employ this method in common use-cases? Thanks.
Hi Javier,
so I've made various small changes we discussed this weeks, and tried to run the test you wrote. and it seems to fail: batch means are still significantly different after applying ComBat(). I pushed the changes to https://github.com/raamana/confounds/tree/combat_param1
can you take a look, and see what's happening? I didn't make any methodological or statistical changes so its unlikely my changes are the issue, but it's possible moving around may have introduced some bugs.
Cheers, Pradeep
Hi @Pradeep,
I've found the bug, corrected it locally passing the tests, but github does not allow me to push the changes. It tells me the following:
remote: Permission to raamana/confounds.git denied to jrasero.
fatal: unable to access 'https://github.com/raamana/confounds.git/': The requested URL returned error: 403
Any idea why this happens?
I edit to ask: I probably need to fork this new branch, make my changes, push it to my github and then open a pull-request, right?
Actually I closed this prematurely @jrasero 😀. We need to expand the test suite to cover many key aspects including the separation between train and test sets and few other checks on gaussianity etc
Hi Javier @jrasero , do you think you might have time for this soon? Cheers.
Hi Javier @jrasero , do you think you might have time for this soon? Cheers.
Sure Pradeep @raamana , I can start working on this next week. Let's me get in contact with you as soon as I am into it.
This pull request includes the implementation of the Combat harmonization method, as originally proposed by Johnson and others in 2007. As of the current implementation, only parametric Bayes frameworks adjustment is covered. I will work on including the non-parametric version shortly.
There are also a couple of functions testing the new estimator, one using simulated data and other using real data. This data was originally used as a test data in the R package SVA that also implements Combat. We then test that the harmonized data using Confounds is the same as that using SVA.