Skip to content

Conversation

uvchik
Copy link
Member

@uvchik uvchik commented Apr 7, 2020

  1. Add function to check the integrity of the turbine data base. This could help to avoid Error in OEP wind_turbine_library causes fails in windpowerlib #100 in the future.
  2. Restructure the data handling and move all related functions into the new data module.

UPDATE:
3. get_turbine_types() will not silently import new database from oedb. In the past the local data has been overwritten by this function.

It might be a good idea to copy some other data-functions to this
module.
@uvchik uvchik self-assigned this Apr 7, 2020
@uvchik uvchik added this to the v0.2.1 milestone Apr 7, 2020
@uvchik uvchik changed the title Add module for data handling Add module for data handling and check integrity of turbine data Apr 24, 2020
@birgits birgits mentioned this pull request May 15, 2020
@uvchik
Copy link
Member Author

uvchik commented May 22, 2020

There are still some tests missing with a wrong dataset.

At the moment the test will log suspicious data sets but will not remove them.
If the check leads to an error the data set will not be stored. This could be changed in the future, so that only broken datasets are remove but the rest will be stored.

It is possible to add additional checks.

@birgits, @SabineHaas Please check whether you agree with the basic approach.

@lgtm-com
Copy link

lgtm-com bot commented May 22, 2020

This pull request introduces 1 alert when merging d32aca5 into 1273dea - view on LGTM.com

new alerts:

  • 1 for Illegal raise

SabineHaas
SabineHaas previously approved these changes Jun 17, 2020
Copy link
Member

@SabineHaas SabineHaas left a comment

Choose a reason for hiding this comment

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

Thank you @uvchik and excuse my late review!

I already give my approval although you should check my suggestions and comments.
I would also appreciate if you could add some more comments and docstrings - at least a one line explanation for each new function. :)


def get_turbine_types(turbine_library="local", print_out=True, filter_=True):
r"""
Get all provided wind turbine types provided.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Get all provided wind turbine types provided.
Get all provided wind turbine types.

"{0}: No cp-curve but has_cp_curve=True.".format(wt_type)
)
if wt.power_curve is not None:
if len(wt.power_curve) < min_pc_length:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a minimum length for cp curves, too?

@uvchik
Copy link
Member Author

uvchik commented Aug 15, 2020

A new thought about this issue. We could store the update of the turbine data at a new place. This will have some advantages:

  • the user do not need the right to change the package
  • the original data set will persist and it is always possible to go back to the original data set
  • it is easier to add/change data without having the risk of making the original data file unreadable

@uvchik uvchik marked this pull request as draft September 23, 2020 08:15
@uvchik uvchik marked this pull request as ready for review November 20, 2020 11:01
@uvchik uvchik requested review from SabineHaas and birgits November 20, 2020 11:01
@uvchik uvchik dismissed SabineHaas’s stale review November 20, 2020 11:03

Now default data is used instead of back files. Please check again.

Copy link
Member

@SabineHaas SabineHaas left a comment

Choose a reason for hiding this comment

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

Looks good to me @uvchik !

filename = os.path.join(os.path.dirname(__file__), "oedb", "{0}.csv")

# get all power (coefficient) curves and save to file
# for curve_type in ['power_curve', 'power_coefficient_curve']:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# for curve_type in ['power_curve', 'power_coefficient_curve']:

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. Thanks 😄

@uvchik uvchik merged commit 1190788 into dev Nov 27, 2020
@uvchik uvchik deleted the features/new_data_structure_with_check branch November 27, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants