8000 Annotations extras by PierreGtch · Pull Request #13228 · mne-tools/mne-python · GitHub
[go: up one dir, main page]
Skip to content

Annotations extras #13228

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 63 commits into
base: main
Choose a base branch
from
Open

Annotations extras #13228

wants to merge 63 commits into from

Conversation

PierreGtch
Copy link
@PierreGtch PierreGtch commented Apr 24, 2025 •
edited by sappelhoff
Loading

Reference issue (if any)

This PR replaces #13213

What does this implement/fix?

The difference with #13213 is that the additional information is a list[dict[str, str | int | float | None]] instead of a dataframe

@PierreGtch PierreGtch marked this pull request as ready for review April 25, 2025 09:36
@PierreGtch PierreGtch mentioned this pull request Apr 25, 2025
Closed
@PierreGtch
Copy link
Author

@drammock I implemented here the recommendations in your #13213 (comment)

drammock
drammock reviewed Apr 25, 2025
View reviewed changes
Copy link
Member
@drammock drammock left a comment

Choose a reason for hiding this comment

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

just a quick first pass with some high-level stuff, LMK when I should take another look @PierreGtch

agramfort
agramfort reviewed Apr 25, 2025
View reviewed changes
Copy link
Member
@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

I am onboard with the change although I am not convinced about the word "details"

@PierreGtch @agramfort @drammock
Apply suggestions from code review
fe6e4a1 
Co-authored-by: Alexandre Gramfort <alexandre.gramfo
8000
rt@m4x.org>
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@PierreGtch PierreGtch changed the title Annotations details Annotations ~details~ extras Apr 26, 2025
@PierreGtch PierreGtch changed the title Annotations ~details~ extras Annotations extras Apr 26, 2025
@PierreGtch
Copy link
Author

All good @drammock :)

@PierreGtch PierreGtch requested a review from drammock May 5, 2025 05:57
@PierreGtch
Copy link
Author

The failed tests seem unrelated to the PR (Read timed out)

drammock
drammock approved these changes May 8, 2025
View reviewed changes
Copy link
Member
@drammock drammock left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this @PierreGtch. I'm pretty happy with where this ended up. Approving (modulo one docstring typo) but would like to wait on merging until at least one other maintainer has had a look (mostly because I've only read the code, haven't actually had time to play around with the feature interactively, and tomorrow I'm off to a conference for a week so I can't do it soon)

@PierreGtch @drammock
Apply review suggestion (spelling)
6de0f4c 
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@PierreGtch
Copy link
Author

thanks @drammock for your time on this PR!

@PierreGtch PierreGtch mentioned this pull request May 9, 2025
Draft
7 tasks
@PierreGtch
Copy link
Author

@sappelhoff @larsoner would you have time to also take a look? (the failed test seems unrelated to the PR)

larsoner
larsoner reviewed May 21, 2025
View reviewed changes
Copy link
Member
@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looks good! Just minor stuff from me really. Can you push the next commit with [circle full] in the commit message? That will make CircleCI build all examples, which I think would be a nice sanity check (and save us some possible post-merge pain in case we missed something)

@drammock
Copy link
Member
drammock commented May 21, 2025 •
edited
Loading

105 CircleCI failures (!). Luckily they're all of 2 types:

  1. TypeError: Annotations.append() takes from 4 to 5 positional arguments but 6 were given
  • examples/datasets/kernel_phantom.py
  • examples/datasets/opm_data.py
  • examples/decoding/decoding_spatio_temporal_source.py
  • examples/decoding/decoding_time_generalization_conditions.py
  • examples/decoding/decoding_unsupervised_spatial_filter.py
  • examples/decoding/decoding_xdawn_eeg.py
  • examples/decoding/ems_filtering.py
  • examples/decoding/linear_model_patterns.py
  • examples/inverse/compute_mne_inverse_epochs_in_label.py
  • examples/inverse/compute_mne_inverse_raw_in_label.py
  • examples/inverse/dics_epochs.py
  • examples/inverse/dics_source_power.py
  • examples/inverse/evoked_ers_source_power.py
  • examples/inverse/mne_cov_power.py
  • examples/inverse/multi_dipole_model.py
  • examples/inverse/multidict_reweighted_tfmxne.py
  • examples/inverse/psf_ctf_vertices_lcmv.py
  • examples/io/elekta_epochs.py
  • examples/preprocessing/css.py
  • examples/preprocessing/define_target_events.py
  • examples/preprocessing/eeg_csd.py
  • examples/preprocessing/eog_artifact_histogram.py 10000
  • examples/preprocessing/eog_regression.py
  • examples/preprocessing/find_ref_artifacts.py
  • examples/preprocessing/ica_comparison.py
  • examples/preprocessing/movement_compensation.py
  • examples/preprocessing/movement_detection.py
  • examples/preprocessing/muscle_ica.py
  • examples/preprocessing/otp.py
  • examples/preprocessing/xdawn_denoising.py
  • examples/simulation/simulate_evoked_data.py
  • examples/simulation/simulate_raw_data.py
  • examples/stats/cluster_stats_evoked.py
  • examples/stats/fdr_stats_evoked.py
  • examples/stats/linear_regression_raw.py
  • examples/stats/sensor_permutation_test.py
  • examples/time_frequency/compute_csd.py
  • examples/time_frequency/compute_source_psd_epochs.py
  • examples/time_frequency/source_label_time_frequency.py
  • examples/time_frequency/source_power_spectrum_opm.py
  • examples/time_frequency/source_power_spectrum.py
  • examples/time_frequency/source_space_time_frequency.py
  • examples/time_frequency/temporal_whitening.py
  • examples/time_frequency/time_frequency_global_field_power.py
  • examples/visualization/3d_to_2d.py
  • examples/visualization/channel_epochs_image.py
  • examples/visualization/eeg_on_scalp.py
  • examples/visualization/evoked_whitening.py
  • examples/visualization/eyetracking_plot_heatmap.py
  • examples/visualization/meg_sensors.py
  • examples/visualization/topo_compare_conditions.py
  • examples/visualization/topo_customized.py
  • tutorials/epochs/10_epochs_overview.py
  • tutorials/epochs/15_baseline_regression.py
  • tutorials/epochs/20_visualize_epochs.py
  • tutorials/epochs/50_epochs_to_data_frame.py
  • tutorials/epochs/60_make_fixed_length_epochs.py
  • tutorials/evoked/10_evoked_overview.py
  • tutorials/evoked/30_eeg_erp.py
  • tutorials/evoked/40_whitened.py
  • tutorials/forward/20_source_alignment.py
  • tutorials/forward/90_compute_covariance.py
  • tutorials/intro/10_overview.py
  • tutorials/intro/15_inplace.py
  • tutorials/intro/20_events_from_raw.py
  • tutorials/intro/30_info.py
  • tutorials/intro/40_sensor_locations.py
  • tutorials/intro/70_report.py
  • tutorials/inverse/30_mne_dspm_loreta.py
  • tutorials/inverse/50_beamformer_lcmv.py
  • tutorials/inverse/70_eeg_mri_coords.py
  • tutorials/inverse/80_brainstorm_phantom_elekta.py
  • tutorials/inverse/95_phantom_KIT.py
  • tutorials/io/70_reading_eyetracking_data.py
  • tutorials/machine-learning/50_decoding.py
  • tutorials/preprocessing/10_preprocessing_overview.py
  • tutorials/preprocessing/15_handling_bad_channels.py
  • tutorials/preprocessing/20_rejecting_bad_data.py
  • tutorials/preprocessing/30_filtering_resampling.py
  • tutorials/preprocessing/35_artifact_correction_regression.py
  • tutorials/preprocessing/40_artifact_correction_ica.py
  • tutorials/preprocessing/45_projectors_background.py
  • tutorials/preprocessing/50_artifact_correction_ssp.py
  • tutorials/preprocessing/55_setting_eeg_reference.py
  • tutorials/preprocessing/59_head_positions.py
  • tutorials/preprocessing/60_maxwell_filtering_sss.py
  • tutorials/preprocessing/90_eyetracking_data.py
  • tutorials/raw/10_raw_overview.py
  • tutorials/raw/20_event_arrays.py
  • tutorials/raw/30_annotate_raw.py
  • tutorials/raw/40_visualize_raw.py
  • tutorials/simulation/70_point_spread.py
  • tutorials/simulation/80_dics.py
  • tutorials/stats-sensor-space/40_cluster_1samp_time_freq.py
  • tutorials/stats-sensor-space/50_cluster_between_time_freq.py
  • tutorials/stats-sensor-space/70_cluster_rmANOVA_time_freq.py
  • tutorials/stats-sensor-space/75_cluster_ftest_spatiotemporal.py
  • tutorials/stats-source-space/20_cluster_1samp_spatiotemporal.py
  • tutorials/stats-source-space/60_cluster_rmANOVA_spatiotemporal.py
  • tutorials/time-freq/10_spectrum_class.py
  • tutorials/time-freq/20_sensors_time_frequency.py
  • tutorials/visualization/10_publication_figure.py
  1. TypeError: Annotations.init() takes from 4 to 6 positional arguments but 7 were given
  • examples/preprocessing/epochs_metadata.py
  • tutorials/clinical/20_seeg.py
  • tutorials/epochs/40_autogenerate_metadata.py

@larsoner
Copy link
Member

Thanks for the quick fix for the bug! For TDD though it's better if we had a test in tests/ somewhere that would have caught that bug / those bugs without needing to run a [circle full] first.... @PierreGtch is it doable to add a test that would have failed before 7406948 ?

@PierreGtch
Copy link
Author

@larsoner thanks for the review!
Is this the kind of test you had in mind?

@PierreGtch
Copy link
Author

The failed test seems unrelated

@drammock
Copy link
Member

@larsoner thanks for the review! Is this the kind of test you had in mind?

Looking at what you had to change in 7406948, I think tests that called __iadd__, _read_annotations_csv, and _read_annotations_fif (either directly, or implicitly via a public API call) should have caught the same failures that running the tutorials caught. My guess is that is what @larsoner meant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner left review comments

@agramfort agramfort agramfort approved these changes

@drammock drammock drammock approved these changes

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

container for eye tracking related annotation information
5 participants
@PierreGtch @sappelhoff @drammock @larsoner @agramfort
0