Skip to content
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

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

Merged
merged 7 commits into from
Apr 28, 2024

Conversation

toni-neurosc
Copy link
Collaborator

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
Copy link
Collaborator Author

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 toni-neurosc merged commit ba4c632 into main Apr 28, 2024
6 checks passed
@toni-neurosc
Copy link
Collaborator Author

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.

@toni-neurosc toni-neurosc deleted the features_protocol branch April 29, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant