-
Notifications
You must be signed in to change notification settings - Fork 19
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
New feature: Custom Semisubmersible design #172
base: dev
Are you sure you want to change the base?
Conversation
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.
Looks good, @nRiccobo! I made one request that mostly has to do with the lack of real testing from the original set of semi submersible tests, otherwise there was just a potential copy/paste error and some general comments for future work.
@@ -66,6 +66,8 @@ semisubmersible_design: | |||
truss_CR: 6250 # USD/t | |||
heave_plate_CR: 6250 # USD/t | |||
secondary_steel_CR: 7250 # USD/t | |||
steel_CR: 4500 # USD/t *CustomSemiSub* | |||
ballast_material_CR: 150 |
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.
Should this have a units comment as well?
substructure = { | ||
"mass": self.substructure_mass, | ||
"unit_cost": self.substructure_unit_cost, | ||
"towing_speed": self._design.get("towing_speed", 6), |
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.
This is a general comment, and I'm recognizing this is an engrained pattern in the code we've already noted, but it'd be really great to move the default values out of the methods themselves.
Custom Semi-Submersible Design Methodology | ||
========================================== | ||
For details of the code implementation, please see | ||
:doc:`Semi-Submersible Design API <api_SemiSubmersibleDesign>`. |
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.
I think this should be referencing the custom version, not the standard version"
@@ -65,3 +69,141 @@ def test_design_kwargs(): | |||
cost = s.total_cost | |||
|
|||
assert cost != base_cost | |||
|
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.
I'm curious if the tests should apply to the each of the various MW rated turbines to ensure the scaling is correct in each case based on a cursory view of the paper. Though, if my understanding was off, and this was done just for the 15 MW, then this all should suffice for a regression test.
@@ -65,3 +69,141 @@ def test_design_kwargs(): | |||
cost = s.total_cost | |||
|
|||
assert cost != base_cost | |||
|
|||
|
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.
As a comment for later work, and not in the near term, I think it'd be good to split out the tests between unit and regression since these are mostly regression tests.
assert custom.substructure_unit_cost == pytest.approx(2.3343e7, 1e-4) | ||
|
||
|
||
def test_custom_design_kwargs(): |
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.
I'm realizing that this was the design pattern for the original set of tests for the semi submersible design, but I was assuming this would also check that the input parameters are being fed through correctly. Would you be able to add basic checks for the new and existing model?
The existing SemiSubmersible() design module originated from a Maness et al. 2017 and used cost curves that scaled all the sub-components by turbine rating. These cost curves are undoubtedly out of date and do not account for different turbine configurations.
The CustomSemiSubmersible() class that was added is based on the a published design of U-Maine's Volturnus design for a 15MW turbine. The model also combines U-Mass's power-law geometric scaling factor to adjust the size of the platform based on rotor diameter (from 15MW to XMW). The user can specify the roughly 12 design variables to size the platform to whatever they want. They can also set the power-law exponent to 0 if they want to avoid it altogether and pass through their own design for any given turbine. Citations included in the model.