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

#181963356 ; fake data alerting #219

Merged
merged 3 commits into from
May 18, 2022
Merged

#181963356 ; fake data alerting #219

merged 3 commits into from
May 18, 2022

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented May 9, 2022

Overview

Added better alerting to inform users when fake data mode is on.

Ticket

https://www.pivotaltracker.com/story/show/181963356

Contributions

  • Added GUI utility for confirmation alerts.
  • Set default parameter for fake_data to False.
  • Added a confirmation alert in bci_main when fake data is present. The user can confirm to proceed with the task or cancel to stop it.

Test

  • Ran all unit tests.
  • Ran a calibration task with the fake_data parameter set to both True and False. Setting to False should not display any alerts. Setting to True should display the alert. Canceling the alert should stop the task. Confirming the alert should proceed.

@lawhead lawhead requested a review from tab-cmd May 9, 2022 19:09
bcipy/main.py Outdated
@@ -95,6 +100,7 @@ def execute_task(task: TaskType, parameters: dict, save_folder: str) -> bool:
language_model = None

fake = parameters['fake_data']
log.info(f"fake data is {'on' if fake else 'off'}")
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to know what fake data means... f"/n fake data is {'on. generating fake data for ' if fake else 'off. attempting to acquire data from'} {device_name}..."

and we could log in the case of fake data and signal model below: f"fake data is on, all decisions will be faked. no signal model required". We talked about making more nuianced fake or simulated behaviour, but this message would alert the user to the current behaviour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion. I moved the logging to the acquisition helper and added more detail. I would like to defer additional logging for fake_decisions for the other ticket.

from PyQt5.QtWidgets import QApplication, QMessageBox


def confirm(message: str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Some duplicate functionality and enums exist in gui_main

It may be useful to use the enums or factor this out of BCIGui to be used elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good callout. I factored out the code for constructing a MessageBox into its own function in GUI/main that's now used in both BCIGui and here. They each call the resulting message box in slightly different ways and have separate use cases, so I think it still makes sense for them to be distinct, but now this module uses functionality provided by the main module.

@lawhead lawhead merged commit 1b2bdfb into 2.0.0rc2 May 18, 2022
@lawhead lawhead deleted the fake-data branch May 18, 2022 16:17
@tab-cmd tab-cmd mentioned this pull request Sep 26, 2022
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