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

Classification ppg #79

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Classification ppg #79

merged 4 commits into from
Dec 3, 2024

Conversation

KarsVeldkamp
Copy link
Contributor

Hierbij de pull request voor de classification stap. Ik verwerk de comments vanuit feature extraction (kleine dingen) in de volgende PR van quantification (heart rate) omdat ik al in die branch actief aan het werk ben.

@@ -19,6 +23,8 @@ def extract_signal_quality_features(df: pd.DataFrame, config: SignalQualityFeatu

# Compute statistics of the spectral domain signals
df_windowed = extract_spectral_domain_features(config, df_windowed)

df_windowed.drop(columns = ['green'], inplace=True) # Drop the values channel since it is no longer needed
Copy link
Contributor

@Erikpostt Erikpostt Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik snap wat je bedoeling is hier, en het scheelt ook code. Maar inplace=True wordt over het algemeen afgeraden:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is het een kwestie van de parameter weghalen? En ben het eens met je laatste comment dat dit miss uberhaupt overbodig is gezien we wss de return aan willen passen!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je moet de transformatie nog wel assignen aan een object, bijv: df_windowed = df_windowed.drop(columns=['green'])

sigma = clf['sigma']

# Prepare the data
lr_clf.feature_names_in_ = ['var', 'mean', 'median', 'kurtosis', 'skewness', 'f_dom', 'rel_power', 'spectral_entropy', 'signal_to_noise', 'auto_corr']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je zou nog kunnen kijken of je deze feature names bij het wegschrijven van de classifier naar pickle mee kan geven. Dan definieer je ze niet twee keer, en kan het ook niet mis gaan als je er één aanpast.

Comment on lines +71 to +73
X_normalized = X.copy()
for idx, feature in enumerate(lr_clf.feature_names_in_):
X_normalized[feature] = (X[feature] - mu[idx]) / sigma[idx]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


# Make predictions for PPG signal quality assessment
df[DataColumns.PRED_SQA_PROBA] = lr_clf.predict_proba(X_normalized)[:, 0]
df.drop(columns = lr_clf.feature_names_in_, inplace=True) # Drop the features used for classification since they are no longer needed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hier geldt hetzelfde als mijn eerdere comment over inplace=True.

df[DataColumns.PRED_SQA_PROBA] = lr_clf.predict_proba(X_normalized)[:, 0]
df.drop(columns = lr_clf.feature_names_in_, inplace=True) # Drop the features used for classification since they are no longer needed

return df
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik denk zelfs uiteindelijk dat we helemaal geen dataframe hoeven te returnen, maar alleen een numpy.array of pandas.Series van de predicted probability. In principe is deze dataframe hetzelfde als de input, met één extra kolom, nietwaar? Ik heb dit zelf ook nog niet geïmplementeerd, dus food for thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Erikpostt Ik denk het ook maar daar moeten we de komende tijd maar even kritisch over nadenken wat het meest handige is.

Copy link
Contributor

@Erikpostt Erikpostt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ziet er goed uit! Merging...

@Erikpostt Erikpostt merged commit cfc2fcb into main Dec 3, 2024
1 check passed
@KarsVeldkamp KarsVeldkamp deleted the classification_ppg branch December 3, 2024 11:05
@Erikpostt Erikpostt assigned KarsVeldkamp and unassigned Erikpostt Dec 3, 2024
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.

2 participants