merge from sb_desal #89
Conversation
fletchapin
force-pushed
the
desal_merge
branch
from
c5df2d1
to
81d748d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
- Coverage 88.92% 88.35% -0.57%
==========================================
Files 16 16
Lines 2393 2895 +502
==========================================
+ Hits 2128 2558 +430
- Misses 265 337 +72 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi Lily! Sorry it took me so long to review but overall this looks good. The major remaining item (besides a couple small suggestions) would be to add tests for all the new classes. Since I'm assuming you're busy with your summer internship I'm happy to take care of that
| @@ -2,7 +2,7 @@ | |||
| from abc import ABC | |||
| from . import utils | |||
| from .tag import Tag, VirtualTag | |||
|
|
|||
| from collections import defaultdict | |||
There was a problem hiding this comment.
I have never used defaultdict so thanks for teaching me something new!
pype_schema/node.py
Outdated
| Type of pump (either VFD or constant) | ||
| Type of pump (either VFD, ERI, AirBlower or constant) | ||
|
|
||
| efficiency : float |
There was a problem hiding this comment.
I'm wondering if this efficiency is duplicative with the pump_curve which represents a similar concept. @LilyLiu0719 could you explain more what the ERI and the new efficiency terms are for exactly?
| @@ -2943,6 +3382,12 @@ class Chlorination(Node): | |||
| volume : int | |||
| Volume of a single chlorinator in cubic meters | |||
|
|
|||
| dosing_rate : dict of DosingType:float | |||
There was a problem hiding this comment.
Since UV is not chlorination, I would prefer creating a parent class Disinfection that includes both Chlorination and UVSystem to distinguish the types of disinfection
| @@ -305,7 +331,7 @@ def __init__( | |||
|
|
|||
| if determine_type: | |||
| if tag_type is not None: | |||
| if tag_type != tag.tag_type: | |||
| if not tag.check_type_compatibility(tag_type): | |||
There was a problem hiding this comment.
I believe there is a bug in this logic. on L315 determine_type is only set to True if tag_type is None. This means that:
if determine_type:
if tag_type is not None:
...
else:
...
is a tautology. I.e., you will always enter the else portion of the statement
pype_schema/utils.py
Outdated
| @@ -491,6 +492,8 @@ class PumpType(Enum): | |||
|
|
|||
| Constant = auto() | |||
| VFD = auto() | |||
| ERI = auto() | |||
There was a problem hiding this comment.
I am changing ERI to ERD (energy recovery device) throughout as I believe this is a typo. Let me know if that's incorrect in some way
| verbose=False, | ||
| ): | ||
| """ | ||
| Incoporates subnetwork (i.e. the `new_network`) into a node |
There was a problem hiding this comment.
Maybe it is just the docstring, but the difference between merge_network and extend_node is not clear to me
Pull request recommendations:
Thanks for contributing!
Change:
node.py, ModularUnit, StaticMixer, ROMembrane, and UVSystem. Modify the corresponding functions inparse_json.pyextend_nodeinparse_json.py, which allow networks incorporates subnetwork into a node in a existing networkDosingTypeEnum class inutils.pyfor chemical dosingcheck_type_compatibilityfunction intag.pyto compare type.