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

CAS-61 Make the APIClient return model objects #81

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Conversation

nmalfroy
Copy link
Collaborator

@nmalfroy nmalfroy commented Sep 5, 2024

This PR makes it so that rather than returning lists of dicts, the API client will now return typed objects. This should make it much easier for users to develop against and understand the CAS api.

Note: this is a breaking change! Results are returned in objects (as opposed to lists of objects) and the list is now in a field named data. This is true for all of the types of enumerations that are returned.

For instance, if an endpoint returns:

[
   {"query_cell_id": "1", ...},
   {"query_cell_id": "2", ...},
]

and is returned as object foo, foo[0] will fail but foo.data will return the list of objects. Furthermore, since these are typed objects, they can't be accessed with subtypes but must be accessed by the variable name.

For instance, in the example above, foo.data[0]["query_cell_id"] will fail but foo.data[0].query_cell_id will return "1".

The docs and example notebook were updated to reflect this

@nmalfroy nmalfroy force-pushed the nm-explicit-models branch 2 times, most recently from 751307c to 1a0bc2b Compare September 5, 2024 15:38
Copy link
Collaborator

@KevinCLydon KevinCLydon left a comment

Choose a reason for hiding this comment

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

This looks great! I have a couple very minor questions, but no objections to merging

Represents the data object returned by the CAS API for nearest neighbor annotations.
"""

class DatasetStatistics(BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly a dumb question: Are these classes nested because they are only used for fields of CellTypeSummaryStatisticsResults?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that's exactly right

Represents the data object returned by the CAS API for a ontology-aware annotations.
"""

class Matches(BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this class represent multiple matches or just one match? If it's just one match, could it be Match?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gah, you're totally right

(e.g. a query of the cell database using a matrix).
"""

class Matches(BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as the other Matches class

@nmalfroy nmalfroy force-pushed the nm-explicit-models branch 2 times, most recently from 03a54ff to 2ab94eb Compare September 6, 2024 13:50
Copy link
Collaborator

@fedorgrab fedorgrab left a comment

Choose a reason for hiding this comment

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

LGTM!

@nmalfroy nmalfroy merged commit 6449c6b into main Sep 9, 2024
5 checks passed
@nmalfroy nmalfroy deleted the nm-explicit-models branch September 9, 2024 15:17
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.

3 participants