-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Calib fix #6
Conversation
…more agnostic to non-numeric calibration IDs
| 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"]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙈
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| 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")) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like that
There was a problem hiding this comment.
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?
| 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")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
…n match string can now be user defined
This makes mark_calibration_as_bad robust to duplicate calibration start/stop triggers, and also makes the regular expression agnostic to the ID of the trigger, i.e. the ID can be any string now, as opposed to any number