Fetch EcephysSession from SLIMS #38
Conversation
| @@ -0,0 +1 @@ | |||
| """Init operations dir""" | |||
There was a problem hiding this comment.
I think we should include the following here so it's like models, and easier to access.
from .ecephys_session import EcephysSession, SlimsEcephysSessionOperator
__all__ = [
"EcephysSession",
"SlimsEcephysSessionOperator",
]
src/aind_slims_api/core.py
Outdated
| @@ -127,7 +127,7 @@ def fetch_models( | |||
| start: Optional[int] = None, | |||
| end: Optional[int] = None, | |||
| **kwargs, | |||
| ) -> list[SlimsBaseModelTypeVar]: | |||
| ) -> List[SlimsBaseModelTypeVar]: | |||
There was a problem hiding this comment.
Super trivial, and I could be wrong but isn't it now preferred to use list over typing.List?
There was a problem hiding this comment.
Ya, since the slims-api requires 3.10 and up, we can use the generic types
| Pydantic model encapsulating all session-related responses. | ||
| """ | ||
|
|
||
| session_group: Optional[SlimsExperimentRunStep] |
There was a problem hiding this comment.
It looks like at least session_group (and maybe session_result?) will always be populated, should they not be Optional?
There was a problem hiding this comment.
there's a (small) chance that even though there is a mouse session run step in a workflow, that session may not exist in the results table. since we don't have too many examples yet of what is/isn't possible I'll leave session_result as optional. Good catch for session_group though
| stimulus_epochs: Optional[List[SlimsStimulusEpochsResult]] = [] | ||
|
|
||
|
|
||
| class SlimsEcephysSessionOperator: |
There was a problem hiding this comment.
Do you see the need for more methods on this class in the future? If it's just fetching the collection, I would be inclined to have that just be a public function in the module.
class EcephysSession(BaseModel):
session_group: Optional[SlimsExperimentRunStep]
...
def fetch_ecephys_session(slims_client: Optional[SlimsClient], subject_id:str) -> list[EcephysSession]:
...If there's the potential for more operations you'd want to do with a session, then maybe I would merge the Operator class with the EcephysSession collection object above, so that it represents a Session concept with relevant attributes and methods.
class SlimsEcephysSessionOperator:
session_group: SlimsExperimentRunStep
...
def __init__(self, slims_client: Optional[SlimsClient], subject_id: str, content_run_pk: int): ...
def end_session(self): ...
def add_step(self, ...): ...
def fetch_sessions(slims_client, subject_id) -> list[SlimsEcephysSessionOperator]:I think I'm skeptical of having to instantiate an operator in addition to the client if it's not going to be a long-lasting helpful object. I know you wanted to have it inherit from SlimsClient which also solves that, though I think that has some drawbacks
| -------- | ||
| >>> from aind_slims_api import SlimsClient | ||
| >>> client = SlimsClient() | ||
| >>> fetch_sessions(client=client, subject_id="000000") |
There was a problem hiding this comment.
Adding an example is great. Following valid doctest syntax may provide added benefit for little cost (makes the example testable against dev). Smallest possible change would be: >>> sessions = fetch_sessions(client=client, subject_id="725804") But isn't a super necessary change.
| return ecephys_sessions | ||
|
|
||
|
|
||
| def fetch_sessions(client: SlimsClient, subject_id: str) -> List[EcephysSession]: |
There was a problem hiding this comment.
This should probably be fetch_ecephys_sessions
closes #30