wind-python / windpowerlib

The windpowerlib is a library to model the output of wind turbines and farms.
https://oemof.org/
MIT License
332 stars 105 forks source link

Refactor v0.2.0 branch #62

Closed uvchik closed 5 years ago

uvchik commented 5 years ago

In my opinion the WindTurbine class was a little bit to complicated even though I like the general idea.

uvchik commented 5 years ago

I could add more tests if you like the general idea.

birgits commented 5 years ago

Thank you very much @uvchik for your suggestions! I think it is alot easier this way. I do have some minor comments/questions:

  1. I would reintroduce the check if either a power curve or power coefficient curve exists, because otherwise no power output calculations are possible. Before the refactoring, an error was raised in that case. However, the curves could also be set at a later point instead of upon initialization of the wind turbine. We could therefore only give out a warning instead of raising an error. @uvchik and @SabineHaas what do you think?
  2. Before the refactoring if no nominal power was given or could be retrieved from the csv file, the maximum power of the power curve was used if possible. I think it is convenient, however, maybe also not very transparent. As it will also probably in most cases not be an issue, as most people will use the oedb library that always has a nominal power when there is a power curve, we could just leave it as it is.
  3. I like that the rotor diameter can now as well be retrieved from the turbine data file. However, the rotor diameter is in many cases a list which will lead to problems in later calculations, which is why we didn't include it yet. Do you have a good suggestions how to handle that?
  4. More tests would be great. I could also add some if you like.
SabineHaas commented 5 years ago

Thank you also from my side @uvchik! I have a minor comment and answer to @birgits:

  1. I would definitely include a check - I think a warning might be enough and advantageous as it gives the possibility of setting a curve later.

  2. If we leave it like that, we should write a sentence somewhere in the docu/docstrings to make this clear as this is important if users want to normalize the feed-in time series - for example oemof has problems with values > 1 in a certain modus.

  3. If it is a list we could give a warning that the rotor diameter could not be set as there are various possibilities, give the possibilities and suggest to set the rotor diameter manually.

uvchik commented 5 years ago

@birgits, @SabineHaas Thank you for your feedback.

  1. What about a special WindpowerlibUserWarning which can be switched off/on globally (see 2297c2f and da59b3d).
uvchik commented 5 years ago
  1. I agree that this is a rare use case but I added a method for this (see a6a692b). You can change the name of the method if you want.
uvchik commented 5 years ago
  1. I could not find any turbine where the rotor_diameter is a list but I agree that we can add a WindpowerlibUserWarning if this is the case.
uvchik commented 5 years ago
* We usually named parameters, variables, etc.. with full names. I'd suggest to  use for example 'power_coefficient_curve' instead of 'pcc' in the code for readability and consistency but I know that this is a matter of taste.

Done with 5634d53 .

uvchik commented 5 years ago
  1. I will start with some tests and see how far I get.
uvchik commented 5 years ago

There are some untested lines in the wind_farm module. Everything else looks pretty okay.

I rebased this PR to the dev branch. It contains all commits of #58, so if we merge this one we can just close the other PR.

uvchik commented 5 years ago

This PR is already very extensive. We should merge it soon if we basically agree with the changes and outsource all pending issues into separate ones. At the moment it becomes very complex and confusing.

uvchik commented 5 years ago

There are a lot of tests packed together in one method/function. Testing is easier if there is just one test in one method/function because at the moment you do not know directly which test has failed if such a packed test fails. Most test do have a comment line this comment line could be the docstring, so I think it does not take more effort to write little tests.

I separate some but not all tests. We can do that step by step because it is not urgent.

You could check 7738df6 or 5cc0685 to see what I mean.

birgits commented 5 years ago

Thank you @uvchik for your great effort, it looks great! The only thing I'd like to discuss before merging is point 2 in my comments below:

  1. I like the windpowerlib warning.
  2. The deduce_nominal_power... method is not yet used anywhere. I would suggest either putting it inside a getter for the nominal power or using it upon initialization of a wind turbine object. I would prefer option number one as it would only be used if needed. I could also implement it if you agree.
  3. My bad, I mixed up the rotor diameter with the hub height. The rotor diameter really never is a list so we can leave it as it is.
  4. Thanks for the tests! I also agree with your comment on splitting tests in the future to make testing easier.
uvchik commented 5 years ago

2) I like explicit more than implicit. So if you want to deduce the nominal_power of the power curve you can do the following:

my_wt = wind_turbine.WindTurbine(**some_data_without_nominal_power)
my_wt.deduce_nominal_power_from_power_curve()
print(my_wt.nominal_power)
birgits commented 5 years ago

I only partly agree. I think that getting the nominal power from the power curve is not that explicit or far-fetched. Furthermore, it is more convenient to only have to provide a power curve and have the nominal power automatically deduced from it in case it is needed for something instead of getting an error message. I talked to @SabineHaas about it and she suggested giving an info in case the nominal power is automatically deduced from the power curve. What do you think about that suggestion?