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

[2.0] Remove DAQ #1189

Open
dbirman opened this issue Dec 9, 2024 · 2 comments
Open

[2.0] Remove DAQ #1189

dbirman opened this issue Dec 9, 2024 · 2 comments
Assignees
Labels
breaking Breaking changes

Comments

@dbirman
Copy link
Member

dbirman commented Dec 9, 2024

Is your feature request related to a problem? Please describe.
DAQ channel mappings are confusing for users

Describe the solution you'd like
We want to remove all references to the DAQ

Additional context
Add any other context or screenshots about the feature request here.

@dbirman dbirman added the breaking Breaking changes label Dec 9, 2024
@bruno-f-cruz
Copy link
Collaborator

(Following the chat in teams...)
In some of our rig configurations, we do keep an optional field for pinouts in sporadic cases. These are mostly used to provide user feedback on the fly) (e.g. if one of the harp boards is unplugged from the main clock. See implementation here https://github.com/AllenNeuralDynamics/Aind.Behavior.Services/blob/d7c30cba05a2bd6b1dac269d9ab411b7d40581c0/src/aind_behavior_services/rig/__init__.py#L276-L280). This information must be entered manually but it is rather light and, critically, optional. If you don't care about extra information, leave it empty.

If removing the daq channels is on the table, perhaps we could discuss alternative solutions before reaching that point:

  1. If you want to remove the connected daq channel information, the user can always push it into the additional_settings.
    additional_settings: AindGenericType = Field(AindGeneric(), title="Additional parameters")
    The point of adding that polymorphic class was to allow people to enter whatever information they deem important and thus extend the already existing class anyway (Consider changing Dict[str,any] of generic fields to any in Calibration model #692). However, it is my understanding that this library is trying to model as much of the setup as possible in an static way, instead of relying on custom extensions by the user (hence Consider changing the default deserialization `extra` mode to `allow` #1113). Did the approach change?
  2. Improve the interface/UX. As suggested previously in (Lists vs Dictionaries when using List[Model] #609 (comment)), small changes to the current daq/daq channel interface could make this class easier to use as well as more useful. This would hopefully lower the adoption cost.
  3. Finally, before removing it altogether, make it an Optional[Collection] with a null default. While this may appear a small change, it allows you to distinguish between a null value (e.g. when people are explicitly not keeping track of any pinouts) and an empty collection (where there simply aren't any daq channels to keep track of).
    Thanks!

@saskiad saskiad added this to the v2.0 milestone Jan 2, 2025
@dbirman
Copy link
Member Author

dbirman commented Jan 6, 2025

The plan is to replace DAQDevice with Connection which stores connections between devices and associated metadata. Something like:

class Connection(AindModel):
    """Connection between two devices"""

    # required fields
    source_device: str = Field(..., title="Name of source device")
    source_channel: Optional[str] = Field(default=None, title="Name of source channel")
    target_device: str = Field(..., title="Name of target device")
    target_channel: Optional[str] = Field(default=None, title="Name of target channel")

@dbirman dbirman self-assigned this Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes
Projects
None yet
Development

No branches or pull requests

3 participants