Closed yalinli2 closed 9 months ago
@yalinli2, @joyxyz1994,
Thanks for making this pull request!
General comment: The new code is bypassing the rest of the code that can allow for more vacuum systems to be defined with preference. The issue with this is that other vacuum systems exist for the same flows/power but for different purposes. What if someone would like to use another vacuum system?
More detailed comments:
Regarding the documentation:
Testing:
Thanks!
@yoelcortes, thanks for the comments!
Regarding your general comment:
Since the original code is forcing F_vol_cfm
to be 3.01, I don't see how it allows user to choose a different vacuum system from the default set. It seems it'll just always choose liquid ring unless DesignError
is returned for P_suction
being too small.
To your detailed comments:
calculate_vacuum_cost
.I think it's a good idea to increase the feed flow rate in the doctest.
Do you have a test function somewhere for the original code? I can add to that.
Thanks!
@joyxyz1994,
Excellent! The old code for flow rate < 3.01 was a place holder... but it might have been better if I had let it error since a good solution was not implemented. It's great that a better solution is being added now.
We can add a TODO for the 2023 CEPCI. I believe I can find it later.
I think it would be a good idea to add the test function in a new module called test_vacuum.py
.
Once you're done making changes, I'll go ahead and make any minor edits and merge :)
Thanks!
@yoelcortes,
New costing algorithm for small vacuum pumps has been added per our discussion above (here). Please review and make edits as you see fit!
Thanks!
Minor updates, one is from @joyxyz1994 on vacuum costs, the other is about the valve graphic, currently an error will be triggered if using the valve and has
graphviz_format
set tosvg
, I don't think the svg issue has been fixed for valves (or maybe I'm not using it correctly...)Thanks!