Skip to content

Calib fix #6

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

Merged
merged 2 commits into from
Jul 21, 2025
Merged

Calib fix #6

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3385b4
make mark_calibration_bad robust to duplicated calibration triggers, …
jshanna100 Jul 18, 2025
7323b97
more informative message for duplicate calibration events. calibratio…
jshanna100 Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions mne_bids_pipeline/steps/preprocessing/_05b_sync_eyelink.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ def _mark_calibration_as_bad(raw):
# marks recalibration beginnings and ends as one bad segment
cur_idx = None
cur_start_time = 0.
last_status = None
for annot in raw.annotations:
calib_match = re.match(".* Recalibration (start|end) \\| (\\d*)", annot["description"])
calib_match = re.match(".* Recalibration (start|end) \\| (.*)", annot["description"])
Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

Could this be an option of the pipeline? E.g. someone else might name their recalibrations differently, or have none at all

Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

maybe something to comment on and for now continue with our lives 🙈

Copy link
Author

@jshanna100 jshanna100 Jul 18, 2025

Choose a reason for hiding this comment

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

how about we make it a user defined string that defaults to what it is now? or shall we make definition mandatory?

Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

sounds very good to me, let's keep our default for now

if not calib_match: continue

calib_status, calib_idx = calib_match.group(1), calib_match.group(2)
if calib_idx == cur_idx and calib_status == "end":
duration = annot["onset"] - cur_start_time
Expand All @@ -51,8 +51,11 @@ def _mark_calibration_as_bad(raw):
elif calib_status == "start" and cur_idx is None:
cur_idx = calib_idx
cur_start_time = annot["onset"]
elif calib_status == last_status:
logger.info(**gen_log_kwargs(message=f"Encountered apparent duplicate calibration event - skipping"))
Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

is it possible to print this calibration match? e.g. it could be recalibration start -> recalibration start or recalibration end -> recalibration end right?

Copy link
Author

@jshanna100 jshanna100 Jul 18, 2025

Choose a reason for hiding this comment

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

recalibration start -> recalibration start or recalibration end -> recalibration end right?

not sure what you mean by this.

Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

Suggested change
logger.info(**gen_log_kwargs(message=f"Encountered apparent duplicate calibration event - skipping"))
logger.info(**gen_log_kwargs(message=f"Encountered apparent duplicate calibration event (last:current => {last_status}:{calib_status})- skipping"))

Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

something like that

Copy link
Author

@jshanna100 jshanna100 Jul 18, 2025

Choose a reason for hiding this comment

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

last_status and calib_status will always per the if condition be the same. could do this instead?

Suggested change
logger.info(**gen_log_kwargs(message=f"Encountered apparent duplicate calibration event - skipping"))
logger.info(**gen_log_kwargs(message=f"Encountered apparent duplicate calibration event ({calib_status}, {calib_idx}) - skipping"))

Copy link
Member

@behinger behinger Jul 18, 2025

Choose a reason for hiding this comment

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

sounds good!

elif calib_status == "start" and cur_idx is not None:
raise ValueError(f"Annotation {annot["description"]} could not be assigned membership")
last_status = calib_status

return raw

Expand Down