-
Notifications
You must be signed in to change notification settings - Fork 21
Adjust srml, Add reference obs documentation #442
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
Conversation
2dc1f9c to
6f63ce5
Compare
wholmgren
left a comment
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. minor comments...
| logger = logging.getLogger('reference_data') | ||
|
|
||
|
|
||
| def adjust_site_parameters(site): |
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 like nearly a copy of the pvdaq function. should we refactor into common?
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.
Yes. Minimal changes would be to move the meat to common with a sitefile kwarg then just call that from pvdaq/srml with the right sitefile
|
There were a handful of warnings related to the new files when building the docs. Check the readthedocs build log or build locally ( |
| def request_data_test(mocker, exception, test_site, test_data): | ||
| get_func = mocker.patch( | ||
| 'solarforecastarbiter.io.reference_observations.srml.iotools.' | ||
| 'read_srml_month_from_solardat') | ||
| get_func.return_value = test_data | ||
| data = srml.request_data(test_site, 1, 1) | ||
| assert_frame_equal(data, test_data) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('exception', [ | ||
| pd.errors.EmptyDataError, | ||
| error.URLError, | ||
| ]) | ||
| def request_data_test_warnings(mocker, exception, test_site): | ||
| logger = mocker.patch( | ||
| 'solarforecastarbiter.io.reference_observations.srml.iotools.' | ||
| 'logger.warning') | ||
| get_func = mocker.patch( | ||
| 'solarforecastarbiter.io.reference_observations.srml.iotools.' | ||
| 'read_srml_month_from_solardat') | ||
| get_func.side_effect = exception() | ||
| data = srml.request_data(test_site, 1, 1) | ||
| assert logger.call_count == 5 | ||
| assert data is None |
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.
Are these tests actually run? They don't start with test_ so I would assume not. Also codecov says no.
wholmgren
left a comment
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.
@alorenzo175 probably worthwhile for you to give this a quick review
…egree tilt portland pv site
Co-authored-by: Will Holmgren <[email protected]>
Co-authored-by: Will Holmgren <[email protected]>
Co-authored-by: Will Holmgren <[email protected]>
Co-authored-by: Will Holmgren <[email protected]>
Co-authored-by: Will Holmgren <[email protected]>
Co-authored-by: Will Holmgren <[email protected]>
|
I factored out the modelling parameter adjustment code into common.py, but I can't figure out what's causing the FileNotFound error in the tests. I've copied the behavior used for the tests in |
|
I'm not seeing the test file at all. Did it git ignored and not committed? |
| site_out = site.copy() | ||
| site_out['modeling_parameters'] = site_metadata[ | ||
| 'modeling_parameters'] | ||
| site_out['extra_parameters'].update(site_extra_params) |
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.
missing from the combined function
| test_json_site_file, | ||
| {'extra_parameters': params}, | ||
| ) | ||
| assert 'modeling_parameters' in new_site |
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.
make sure extra_parameters are updated
docs/source/api.rstfor API changes.docs/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).This PR does a few things: