Add New Data Types for Simularium Frame Data #157
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 92.56% 93.05% +0.49%
==========================================
Files 101 106 +5
Lines 4546 4941 +395
==========================================
+ Hits 4208 4598 +390
- Misses 338 343 +5
☔ View full report in Codecov by Sentry. |
ascibisz
requested review from
blairlyons,
frasercl,
interim17,
meganrm and
toloudis
and removed request for
a team
| """ | ||
| Return frame data for frame at index. If there is no frame at the index, | ||
| return None. | ||
| """ |
There was a problem hiding this comment.
Not sure why I didn't notice this before but are you supposed to put pass in each of these functions? Also it looks like this is supposed/intended tobe an abstract base class
from abc import ABC
class MyABC(ABC):
...
@abstractmethod...
Is that sort of the intention here?
There was a problem hiding this comment.
autoflake doesn't like it when you have pass after a docstring, so maybe I'll move the docstrings to the subclasses? I hadn't even noticed that pass had been removed, it just disappeared when I ran autoflake locally. Good eye
|
|
||
|
|
||
| class SimulariumFileData: | ||
| def __init__(self, file_contents: Union[str, bytes]): |
There was a problem hiding this comment.
maybe the base class doesn't need to have the file_contents even as a constructor argument?
toloudis
left a comment
toloudis
left a comment
There was a problem hiding this comment.
Overall lgtm, I added a couple of minor comments on the details of the base class but nothing serious.
Problem
It'd be nice to have simularium data objects that are designed for providing frame data for a trajectory, so that the octopus repo doesn't have to define it's own. Should match what is defined in this miro board under "New SimulariumIO Data Types (pseudo code)"
Ticket
There will be a forthcoming PR for using these new data objects in Octopus
Type of change