search

neuromodulation / py_neuromodulation

Real-time analysis of intracranial neurophysiology recordings.
https://neuromodulation.github.io/py_neuromodulation/
MIT License
41 stars 9 forks source link

Turn Feature abstract class into a Protocol and lazy load feature modules #307

Closed toni-neurosc closed 2 months ago

toni-neurosc commented 2 months ago

I noticed that the Preprocessing classes were implemented as a protocol instead of an abstract class. I thought it would be a good idea to do the same with the Feature class

I also renamed NMFeature for clarity, Feature seemed a very generic word that is repeated many times over the codebase.

I took the opportunity to change the Features class (maybe rename this one too?) initialization to only import the feature modules when the feature is to be calculated, that way there are no unnecessary imports and the module already takes forever to initialized (>3 seconds just for the startup).

One issue with this changes is that numpy is complaining about "divisions by zero" during feature normalization

RuntimeWarning: invalid value encountered in divide
  current = (current - nan_mean(previous, axis=0)) / nan_std(previous, axis=0)

But I checked and the zeroes were present already, but for some reason the changes triggered the printing of the warning. I'll check if the output is the same, and if it is I'll try to suppress the warning.

But are there supposed to be divisions by zero anyway? Seems kind of suspicious to have nan or 0 standard deviation.

toni-neurosc commented 2 months ago

Ok, so somehow it's not throwing the warnings anymore on my computer for some reason.

Anyway, those warnings are for 0/0, non-zero/0 gives RuntimeWarning: divide by zero encountered in divide instead.

0/0 in numpy gives a nan result, so for the purposes or normalization I'm not sure if that's a problem the offending line is:

        case NORM_METHODS.ZSCORE.value:
            # if denominator and numerator both 0, current = nan
            current = (current - nan_mean(previous, axis=0)) / nan_std(previous, axis=0)

If you had already taken that into account, please ignore this message altogether.

toni-neurosc commented 2 months ago

Ok so the problem I mentioned in the previous comment happened because I was not testing the settings properly apparently. It's not happening now and I also incorporated the changes made in #317 to this PR, and I'm merging it in.