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

Add timer for surface finding #1228

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Conversation

XX-Yin
Copy link
Collaborator

@XX-Yin XX-Yin commented Dec 23, 2024

Pull Request instructions:

  • Please follow the update protocol
  • Answer the questions below in detail. Your responses will be emailed to experimenters.
  • If the experimenters must do anything new, provide detailed step by step instructions on the wiki
  • If computer maintainers need to manually update anything, provide detailed step by step instructions
  • Use markdown syntax in order for your comments to be rendered reliably in the email: "1." instead of "1)", use four spaces for indents.
  • If you use the keyword "skip email" in the title, it will skip the email updates
  • Merges from "develop" into "production_testing" should use the keyword "production merge" in the title for reliable indexing of updates
  • Merges from "production_testing" into "main" should use the keyword "update main"

Describe changes:

Add a timer for ephys recording when the condition type is set to "surface finding." The recording will automatically prompt the user for confirmation to terminate once the timer ends. Additionally, the user will have the option to manually stop the recording at any time during the session.

What issues or discussions does this update address?

It's more convenient to have a timer for the "surface finding".

Describe the expected change in behavior from the perspective of the experimenter

No

Describe any manual update steps for task computers

No

Was this update tested in 446/447?

Tested in 323_Ephys3

@alexpiet
Copy link
Collaborator

This seems like an ephys specific feature, and not a behavior feature.

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Dec 23, 2024

I would say they are behavior+ephys features. We are using the GUI to control the start/stop of OpenEphys GUI. The advantage is that we can retrieve the settings from OpenEphys, which are used to generate the session metadata.

@alexpiet
Copy link
Collaborator

We need a clear plan for what ephys+behavior features are going to be in the GUI, and what will be dedicated software. We are adding significant complexity to the GUI to handle unusual ephys situations that would be easier to maintain in a separate piece of software

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Dec 23, 2024

We need a clear plan for what ephys+behavior features are going to be in the GUI, and what will be dedicated software. We are adding significant complexity to the GUI to handle unusual ephys situations that would be easier to maintain in a separate piece of software

@alexpiet

  1. The new function adds a few simple lines to control the start/stop of Open Ephys, which is straightforward and similar to the timer used in the fiber photometry baseline signal.

  2. The primary reason for using the GUI to control Open Ephys is metadata generation. By doing so, we can automatically retrieve the settings from Open Ephys for that session and generate the metadata seamlessly. While I’m open to an independent solution without the GUI—such as what @KanghoonJ is working on with SIPE—the timeline for that is uncertain. We shouldn’t let the pursuit of a "perfect" solution hinder our current experiments, especially since we don’t know if it will indeed be perfect. Otherwise, we’ll be unable to obtain session metadata for ephys recordings, preventing us from uploading and analyzing the data.

@alexpiet
Copy link
Collaborator

I'm not asking for a perfect solution - I want a plan for a solution. If we can't have stand-alone ephys control (or its slow to be available), then we need a plan for how ephys and behavior interact. Otherwise the code gets messy and hard to maintain very quickly.

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Dec 24, 2024

@alexpiet

I feel that the influence of this PR might be a bit overstated. The functionality is quite straightforward, similar to a timer used in photometry or the start/stop functions for photometry or a camera. While these standalone systems (e.g., Spinnaker software) have their own controls, they still need to communicate with the behavior GUI for synchronization.

What we’re doing here follows the same principle. The ephys system remains standalone, but we’re using the behavior GUI to control its start/stop and to extract parameters for metadata generation.

Were you perhaps referring to a different aspect? I’m having difficulty understanding how such a simple function could make the GUI gets messy and hard to maintain very quickly.

Additionally, what kind of plan do you want? Who do you think we should involve in a discussion to ensure we approach this issue effectively?

@alexpiet
Copy link
Collaborator

alexpiet commented Dec 24, 2024

Its not this PR alone, its the multiplicative effect of new features that are added without a larger plan. For example your other open PR about disabling manifest upload for ephys. Bugs get created when this little features don't align properly. We haven't been planning things out well, and as a result we are buried under technical debt. Lets try to improve our software development going forward. Note that I'm trying to impose this same degree of planning on the rest of the GUI too. Micah is working on adopting the data models from Aind.Behavior.Services, to help organize the code.

A plan looks like a written document of how the GUI and ephys tools should interact. Detailing:

  • Who maintains these features? Ephys rigs are highly customizable, we can't be in a position where we have complex code that covers all ephys configurations.
  • What is the specific data model that describes when ephys recordings start/stop
  • Feedback from all ephys users would be valuable.

@XX-Yin
Copy link
Collaborator Author

XX-Yin commented Dec 24, 2024

Its not this PR alone, its the multiplicative effect of new features that are added without a larger plan. For example your other open PR about disabling manifest upload for ephys. Bugs get created when this little features don't align properly. We haven't been planning things out well, and as a result we are buried under technical debt. Lets try to improve our software development going forward. Note that I'm trying to impose this same degree of planning on the rest of the GUI too. Micah is working on adopting the data models from Aind.Behavior.Services, to help organize the code.

A plan looks like a written document of how the GUI and ephys tools should interact. Detailing:

  • Who maintains these features? Ephys rigs are highly customizable, we can't be in a position where we have complex code that covers all ephys configurations.
  • What is the specific data model that describes when ephys recordings start/stop
  • Feedback from all ephys users would be valuable.

I understand your concerns, and I’m open to discussing specific ways to improve the system.

I believe this concern is being overemphasized. These features are straightforward and unrelated to the functionality of the ephys software. They simply serve as start/stop functions, much like those used for photometry and camera control.

If this level of concern applies here, do you also have plans to propose updates to the photometry and camera systems for consistency?

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