-
Notifications
You must be signed in to change notification settings - Fork 135
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
Output Reader abstraction #511
Conversation
@karosc If you could review this one first, that would be a little better with the code flow. I meant to branch of develop-v2 for my other PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes largely look fine to me. My only comments are related to developer experience and style choices.
Things I like
- Concise property based access of timeseries data
- Similar usage to
Nodes(sim)
,Links(sim)
,Subcatchments(sim)
objects, which provides a cohesive feel to the pyswmm coding style with both simulation and output objects - docstrings 🤤
- Nearly full coverage of new code with new unit tests (technically the exceptions in the
OutAttributeBase
class are not tested, but we can let that slide probably 😉)
Things I would improve
- Missing type hints. This is not a precedent set in this repo, so I can accept not having them. But in my opinion, python code without type hints in this era is incomplete code. We should consider adding type stubs to inline hints as an improvement in the future. Because, they are not included here now, we will have to create them later if we take on that effort.
- Although you can access timeseries as props with these changes, they are not defined in a
__dir__
method and so you cannot tab complete them. Providing the timeseries data access as properties is a developer experience improvement, so why don't we take it that extra step to get tab completion working so users don't need to have docstrings open side-by-side with code to know what props are available to each SWMM object.
@karosc exceptional review! I’ll make all the changes you’ve requested. I’ll plan to do it before the end of the week if you can be available again on Friday for round 2. |
@karosc, Ready for review / approval! |
So it turns out we might need to add type hints for IDEs to recognize the props. The __dir__method will allow autocomplete in a REPL, but the IDE's and static type checkers wont read it. Maybe we can add some property type hints at the top of each class comme ça? class LinkSeries(OutAttributeBase):
"""
Get a link time series. New to PySWMM-v2!
Note: you can use pandas to convert dict to a pandas Series object with dict keys as index
:return: dict of attribute values with between start_index and end_index
with reporting timesteps as keys
:rtype: dict {datetime : value}
Examples:
.. code-block:: python
from pyswmm import Output, LinkSeries
with Output('tests/data/model_full_features.out') as out:
ts1 = LinkSeries(out)['C1:C2'].flow_rate
ts2 = LinkSeries(out)['C1:C2'].flow_depth
ts3 = LinkSeries(out)['C1:C2'].flow_velocity
ts4 = LinkSeries(out)['C1:C2'].flow_volume
ts5 = LinkSeries(out)['C1:C2'].capacity
ts6 = LinkSeries(out)['C1:C2'].pollut_conc_0
"""
flow_rate: dict[datetime.datetime, float]
flow_depth: dict[datetime.datetime, float]
def __init__(self, out_handle):
super().__init__(out_handle)
self._attr_group = shared_enum.LinkAttribute
self._idname = None
def __getitem__(self, idname):
self._idname = idname
return self
def _series_type(self, attr_select):
return self._handle.link_series(self._idname, attr_select)
|
Closes #281, #407
I took this opportunity to build upon what we have in here to remove the need to go to swmm.toolkit to investigate the enum attributes to find what properties are available. I think that made it overly difficult to use... So I fixed it. This now done with properties. As we / if we add new properties to the ENUM later, we will not need to modify anything (but we can update docs if needed).
Example of this is used: