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.

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

Model Loading #355

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Model Loading #355

merged 3 commits into from
Oct 24, 2024

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented Oct 19, 2024

Overview

Changed model loading behavior to prompt the user using a separate file dialog for each model.

Ticket

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

Contributions

  • Updated the ask_filename utility to optionally take a prompt. Also updated to use the PyQt file dialog, rather than the native dialog; this provides consistency between platforms as they don't always display the prompt.
  • Added utility functions to the acquisition helper for getting the active device content types based on the configured acq_mode.
  • Added load functions for models.

Test

  • Created unit tests for new functions.
  • Initialized Copy Phrase task with different acq_mode configurations to confirm that the file prompts were working correctly.

@lawhead
#188038501 ; Changed model loading behavior to prompt using a separat…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
259f8ef 
…e file dialog for each model
@lawhead lawhead requested a review from tab-cmd October 19, 2024 01:02
lawhead
lawhead commented Oct 19, 2024
View reviewed changes
bcipy/gui/file_dialog.py
@@ -25,18 +27,21 @@ def __init__(self):

# The native dialog may prevent the selection from closing after a
# directory is selected.
self.options = QFileDialog.Option(3)
self.options = QFileDialog.Option.DontUseNativeDialog
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this was previously broken because the Option enum changed between pyqt5 and 6.

tab-cmd
tab-cmd reviewed Oct 20, 2024
View reviewed changes
Copy link
Contributor

@tab-cmd tab-cmd left a comment

Choose a reason for hiding this comment

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

Let's wait on the orchestrator merge; this will change where it needs to be implemented (task vs bci_main). The PR is ready.

@lawhead
Copy link
Collaborator Author

lawhead commented Oct 24, 2024

Let's wait on the orchestrator merge; this will change where it needs to be implemented (task vs bci_main). The PR is ready.

The orchestrator changes have been merged into this branch and the model loading code was moved to the task. This PR should be ready for review.

@tab-cmd tab-cmd self-requested a review October 24, 2024 20:07
tab-cmd
tab-cmd approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@tab-cmd tab-cmd left a comment

Choose a reason for hiding this comment

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

It works well for me! Thanks for making the updates after the orchestrator merge.

@lawhead lawhead merged commit 862e3e0 into 2.0.0rc4 Oct 24, 2024
6 checks passed
@lawhead lawhead deleted the model-loading branch October 24, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tab-cmd tab-cmd

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

Successfully merging this pull request may close these issues.
2 participants
@lawhead @tab-cmd