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

Restructure folders #70

Merged
merged 24 commits into from
Oct 15, 2024
Merged

Restructure folders #70

merged 24 commits into from
Oct 15, 2024

Conversation

KarsVeldkamp
Copy link
Contributor

I have restructured the paradigma src folder. Every domain now has his own subdirectory in which every step of the pipeline can be implemented.

@@ -9,9 +9,18 @@
},
{
"cell_type": "code",
"execution_count": 1,
"execution_count": 73,
Copy link
Contributor

Choose a reason for hiding this comment

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

Deze heeft nog een execution count uitstaan. Je kan bovenin de notebook "restart" en "clear_output" klikken, dan is het helemaal opgeschoond.

Comment on lines 19 to 20
"The autoreload extension is already loaded. To reload it, use:\n",
" %reload_ext autoreload\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

De autoreload zou ik gebruiken voor als je zelf codeert in notebooks, maar niet voor notebooks die gebruikers gaan draaien of die gebruikt worden in docs. Dan is het reloaden overbodig omdat het draait op de release branch en niet op lokale veranderingen.

"metadata": {},
"outputs": [],
"source": [
"heart_rate_analysis_config import HeartRateFeatureExtractionConfig, HeartRateClassificationConfig\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Volgens mij gaat hier iets niet helemaal goed.

@@ -65,30 +86,15 @@
"metadatas_ppg, metadatas_imu = scan_and_sync_segments(os.path.join(path_to_sensor_data, 'ppg'),\n",
" os.path.join(path_to_sensor_data, 'imu'))\n",
"preprocess_ppg_data(metadatas_ppg[0], metadatas_imu[0],\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Waarom gebruik je hier alleen de 1e index? Als je dit vaker wilt gebruiken zou je de functie ook wellicht kunnen draaien met alleen de 1e index als uitkomst, in plaats van voor alle data te draaien en dan alleen de 1e index te pakken?

"metadata": {},
"outputs": [],
"source": [
"#TODO: Not implemented\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Deze werkt wel (een beetje)? Of moet dit ook uitgecommend worden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uitgecommend idd

def write_data(metadata_time: TSDFMetadata, metadata_samples: TSDFMetadata,
output_path: str, output_filename: str, df: pd.DataFrame):

def write_np_data(
Copy link
Contributor

Choose a reason for hiding this comment

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

Waarom wil je het als numpy wegschrijven in plaats van pandas?

Comment on lines +33 to +34
epoch_length = 6 # in seconds
overlap = 5 # in seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Deze kan je het beste ook in de constants ergens definiëren

Copy link
Contributor

Choose a reason for hiding this comment

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

Ik zie dat je overal met numpy werkt, is dat handiger dan met pandas?

Comment on lines +37 to +47
samples_per_epoch_ppg = int(epoch_length * sampling_frequency_ppg)
samples_per_epoch_acc = int(epoch_length * sampling_frequency_imu)

# Calculate number of samples to shift for each epoch
samples_shift_ppg = int((epoch_length - overlap) * sampling_frequency_ppg)
samples_shift_acc = int((epoch_length - overlap) * sampling_frequency_imu)

pwelchwin_acc = int(3 * sampling_frequency_imu) # window length for pwelch
pwelchwin_ppg = int(3 * sampling_frequency_ppg) # window length for pwelch
noverlap_acc = int(0.5 * pwelchwin_acc) # overlap for pwelch
noverlap_ppg = int(0.5 * pwelchwin_ppg) # overlap for pwelch
Copy link
Contributor

Choose a reason for hiding this comment

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

Kijk even goed naar de afronding dmv int: vaak kan je beter eerst .round() doen, wat van een 0.6 een 1 maakt, ipv int, wat van een 0.6 een 0 maakt.

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.

Hey, ziet er goed uit! Kan je even de comments nalopen, kijken of je nog iets wilt aanpassen voor deze PR, en mij dit laten weten? Als er niets aangepast hoeft te worden voor deze PR merge ik 'm.

@Erikpostt Erikpostt assigned KarsVeldkamp and unassigned Erikpostt Oct 15, 2024
@KarsVeldkamp
Copy link
Contributor Author

Heb de comments nagelopen, paar hele kleine dingen aangepast. De rest wordt nog aangepast allemaal gerefactord en ga ik in deze PR niet doen om het overzichtelijk te houden ;)

@Erikpostt
Copy link
Contributor

@KarsVeldkamp nog één commit doen: clear_output moet je nog even doen voor de notebook.

@Erikpostt Erikpostt assigned KarsVeldkamp and unassigned Erikpostt Oct 15, 2024
@KarsVeldkamp
Copy link
Contributor Author

@KarsVeldkamp nog één commit doen: clear_output moet je nog even doen voor de notebook.

Dat heb ik toch gedaan in de laatste commit? Check de execution counts etc ;) Of begrijp ik je verkeerd?

@Erikpostt
Copy link
Contributor

@KarsVeldkamp nog één commit doen: clear_output moet je nog even doen voor de notebook.

Dat heb ik toch gedaan in de laatste commit? Check de execution counts etc ;) Of begrijp ik je verkeerd?

Je mist nog één cel ;)

@KarsVeldkamp
Copy link
Contributor Author

hmm gek, want kon hem net niet opnieuw clearen. Nu wel nog een poging gedaan alleen begint hij nu bij 1 i.p.v. null

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.

Looking good, merging after conflicts were solved.

@Erikpostt Erikpostt merged commit cbf031c into main Oct 15, 2024
1 check passed
@Erikpostt Erikpostt deleted the restructure_folders branch October 15, 2024 13:08
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.

3 participants