search

ME-ICA / mapca

A Python implementation of the moving average principal components analysis methods from GIFT
GNU General Public License v2.0
6 stars 8 forks source link

Scaler parameters overridden within MovingAveragePCA #30

Closed tsalo closed 3 years ago

tsalo commented 3 years ago

@eurunuela in #26, it looks like you commented out code that generated a Scaler within the subsampling procedure, and instead reused the one for the general scaling:

https://github.com/ME-ICA/mapca/blob/6c1d58c0256660561a96c0183d4e477b4fd16c4e/mapca/mapca.py#L174-L177

My concern is that re-fitting self.scaler_ based on new data will make it invalid for rescaling data back to the original scale, as here:

https://github.com/ME-ICA/mapca/blob/6c1d58c0256660561a96c0183d4e477b4fd16c4e/mapca/mapca.py#L331-L332

Does that make sense, or was there a specific reason you got rid of the original approach? Apologies for not noticing this in #26 and bringing it up while that PR was open.

eurunuela commented 3 years ago

The reason I commented that line out is the scaler object is already generated at the start of the function. I didn't see the need to generate the same object again (same parameters).

tsalo commented 3 years ago

Ah, okay. I think the problem is real then. Namely, inverse_transform will assume the mean and standard deviation in self.scaler_ correspond to the original data, but they will really correspond to the sub-sampled data defined in that if statement. I can open a PR to address this.