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

Vacuum costing and valve graphic fix #172

Merged
merged 26 commits into from
Feb 13, 2024
Merged

Vacuum costing and valve graphic fix #172

merged 26 commits into from
Feb 13, 2024

Conversation

yalinli2
Copy link
Member

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 to svg, I don't think the svg issue has been fixed for valves (or maybe I'm not using it correctly...)

Thanks!

Copy link
Collaborator

@joyxyz1994 joyxyz1994 left a comment

Choose a reason for hiding this comment

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

Hey @yoelcortes, I updated the costing function for small vacuum pumps with F_vol_cfm < 3.01 using data I found for commercial vacuum pumps with 3 cfm and 1.5 cfm design capacities here. It seems by previous assumptions, when F_vol_cfm < 3.01, the design and cost of the vacuum pumps would be fixed at F_vol_cfm = 3.01, which would disproportionally inflate the costs of very small systems. Do you think the new assumption makes sense? Can we update the doctest accordingly?

@yoelcortes
Copy link
Member

yoelcortes commented Sep 28, 2023

@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:

  • Vacuum costing code should be in calculate_vacuum_cost. Maybe you can add a new option for the grade/system (e.g. system='rotary vane' and grade='single stage')?
  • The work should be based on the suction of air. I see work is a constant. Is this an upper limit for the vacuum? If so, could you add this to the # %% Data section (once you defined a new system and grade)?
  • The CEPCI is for 2021 while the cost is based on 2023.
  • More detail in the reference for the vacuum system cost would be nice. It would be great to have it with a similar standard as you would reference it in a paper.

Regarding the documentation:

  • We can increase the feed flow rate in the doctest to make sure the tests are passing and keep the coverage for the old code.

Testing:

  • Can we include a unit test (a test function; not a doctest) within the tests folder? This helps protect your code from being altered by any future changes (e.g. someone introduces another vacuum).

Thanks!

@joyxyz1994
Copy link
Collaborator

@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:

  • I'll add a new vacuum system option and move the code to calculate_vacuum_cost.
  • Work is a constant and treated as an upper limit, because it's based on product specifications I can find about commercial vacuum pumps out there. Will add it to the data code block.
  • If you have access to 2023 CEPCI, I'm happy to update that.
  • I can move the reference to the specific function for this calculation.

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!

@yoelcortes
Copy link
Member

yoelcortes commented Sep 29, 2023

@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!

@joyxyz1994
Copy link
Collaborator

@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!

Copy link
Member

@yoelcortes yoelcortes left a comment

Choose a reason for hiding this comment

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

Amazing work! Love the tests. Feel free to add your name to the copyright in the module when making significant changes, otherwise I'll add it later.

Thanks!

@yoelcortes yoelcortes merged commit a0d0aaa into master Feb 13, 2024
1 of 2 checks passed
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