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

Add power curve functionality #377

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

cpjordan
Copy link
Contributor

@cpjordan cpjordan commented Dec 4, 2024

Adds the option to provide power curves alongside thrust curves. Previously the power coefficients would have been determined based on the thrust coefficients as per the Martin-Short et al., 2015 paper. However, if the power coefficient data is available, we will want to use it directly.

Also updated the tidal turbine array example to demonstrate how to use different turbine types, as that functionality had been added in #358 but not demonstrated.

cpjordan and others added 8 commits November 27, 2024 16:58
- Until Firedrake issue #3368 is resolved
- Interested in scalar/vector/tensor not DOF structure
- Also simplify to fs.value_shape - no need for ufl_element().value_shape
- If a power coefficient table is not provided, then behaviour defaults to the Martin-Short et al., 2015 equation as previously
- Fix the Thetis options for power curve functionality as well
@cpjordan cpjordan marked this pull request as ready for review December 5, 2024 09:31
Comment on lines 169 to 171
for i in range(len(cb_turbines_AR1500.variable_names)):
print_output(cb_turbines_AR1500.variable_names[i] + ': (' + str(cb_turbines_AR1500.get_turbine_coordinates[i][0])
+ ', ' + str(cb_turbines_AR1500.get_turbine_coordinates[i][1]) + ')')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for i in range(len(cb_turbines_AR1500.variable_names)):
print_output(cb_turbines_AR1500.variable_names[i] + ': (' + str(cb_turbines_AR1500.get_turbine_coordinates[i][0])
+ ', ' + str(cb_turbines_AR1500.get_turbine_coordinates[i][1]) + ')')
for i, name in enumerate(cb_turbines_AR1500.variable_names):
print_output(f'{name}: ' '({cb_turbines_AR1500.get_turbine_coordinates[i][0]}, '
'{cb_turbines_AR1500.get_turbine_coordinates[i][1]})')

Copy link
Contributor

Choose a reason for hiding this comment

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

Not important, but these bla + ': (' + str(...) + ' , ' + str(...) + ') type of things are a little hard to parse so I prefer format strings there. Same in two more loops below.



class ConstantThrustTurbine(TidalTurbine):
def __init__(self, options, upwind_correction=False):
super().__init__(options, upwind_correction=upwind_correction)
self.C_T = options.thrust_coefficient
self.C_P = getattr(options, "power_coefficient", 0.5 * self.C_T * (1 + (1 - self.C_T) ** 0.5))
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems you are trying to make it such that if "power_coefficient" is not specified it uses the analytical formula (as before I guess). However "power_coefficient" will always be present in ConstantTidalTurbineOptions with a default value of 0.7. So if the user only sets the thrust coefficient, say to some low value of 0.5, then the power coefficient will still be set to 0.7. If you want to make setting of power_coefficient turbine optional - one way to do it would be to make it a PositiveFloat(None, allow_none=True, "explain default None in help") and then self.C_P = options.power_coefficient or 0.5 * self.C_T * (1 + (1 - self.C_T) ** 0.5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you! I've applied the same to the tabulated approach as well for consistency

# this assumes the velocity through the turbine does not change due to the support (is this correct?)
return 0.25*C_T*A_T*(1+sqrt(1-C_T))*uv3
return 0.5*(physical_constants['rho0']/1000)*A_T*C_P*uv3
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense, but requires a comment. I presume we are outputting energy in kW then? Basically the random factor of a 1000 needs explaining. Also would be happy if you decide to change it to output in W instead (so we're consistently SI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had kept it consistent with previously in case there was a reason for this. I agree with changing it to Watts. Inclusion of the density is necessary as most thrust and power curves expect it to be 1025-1026kg/m3, not 1000kg/m3.

Comment on lines 483 to 486
thrust_coefficients = List([3.0], help='Table of thrust coefficients')
power_coefficients = List([2.7], help='Table of power coefficients')
thrust_speeds = List(
[0.8, 0.8],
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the default values for these are wrong (were wrong already!) with speeds and coefficients swapped. Also they don't pass the new check that the list of thrust coefficients and speeds should be the same length.

Copy link
Contributor Author

@cpjordan cpjordan Dec 19, 2024

Choose a reason for hiding this comment

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

Thought they might have just been exaggerated to show the influence of the turbine more clearly, but this makes more sense.

- Farm power units to W (from kW)
- Do not require C_P specification
- Update print statement formatting
Copy link
Contributor

@thangel thangel left a comment

Choose a reason for hiding this comment

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

It looks like all requested changes were addressed so approving this - it looks good!

Copy link
Contributor

@stephankramer stephankramer left a comment

Choose a reason for hiding this comment

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

Yes, all looks good! Great stuff

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