Repository: NREL/rdtools (language: python)

Issue:
Update github actions triggers
- ~[ ] Code changes are covered by tests~
- ~[ ] Code changes have been evaluated for compatibility/integration with TrendAnalysis~
- ~[ ] New functions added to `__init__.py`~
- ~[ ] API.rst is up to date, along with other sphinx docs pages~
- ~[ ] Example notebooks are rerun and differences in results scrutinized~
- [x] Updated changelog

Closes #312
Docstring standardization and pep8 formatting
Referencing #122.  This PR has gotten a lot bigger than I anticipated.  High-level overview: 

1) Reformatted all docstrings to be compatible with the numpy docstring format for use with the [Napoleon Sphinx extension](https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html) in anticipation of setting up RdTools on readthedocs.io.  

2) I also reformatted much of the code to not be over 80 characters long in anticipation of setting up a pep8 linter.  In most cases this just involved breaking code over several lines, but in a few cases it involved "real" code changes like introducing temporary variables.  

3) Added a bunch of sphinx files so that fancy RTD-style docs are autogenerated from the code:
    - The main page is generated from README.md.  Had to make a few formatting changes to allow that translation to work)
    - An example page generated from the example ipython notebook.  Had to make all the titles except for the first one smaller (level 2 instead of level 1).  Will be easy to add additional notebooks later on as other branches get merged.  
    - An API reference that includes submodule summaries, function summaries, and full function documentation.  

To-Do lists
-----------
Docstrings:

- ~clearsky_temperature.get_clearsky_tamb has undocumented parameters~
- ~degradation.* function descriptions lack detail~
- All module descriptions lack detail
- ~filtering.poa_filter is undocumented~
- ~filtering.tcell_filter is undocumented~
- ~normalization.check_series_frequency is poorly documented~
- ~normalization.delta_index is poorly documented~
- ~normalization.ConvergenceError is undocumented~
- ~typo in multiple right-labeled notices~
- ~typo in irradiance_rescale/method~

Sphinx configuration:

- ~rdtools\docs\sphinx\source\modules.rst: WARNING: document isn't included in any toctree~
- ~WARNING: html_static_path entry 'rdtools\docs\sphinx\source\_static' does not exist~
- ~What should the copyright be?  Currently have `© Copyright 2019, National Renewable Energy Laboratory, kWh Analytics, Github contributors`~
- make sure the makefile is equivalent to the custom build script
- ~spruce up the index page, maybe copy over the README.md content.  linking `README.md` with `index.rst` almost works well with a redirect rst file and the `m2r` package, but images don't work because the paths don't get translated.  Might need to rewrite the readme in RST to make it work.~
- add link to https://www.nrel.gov/pv/rdtools.html?
- ~make a page for each module~
- explanations of the functional and OO interfaces -- I vote to move this to a new PR or #117
- ~most of the links in README.md don't work after translation.  Probably there's a way to modify them to work in both the MD file on github and the generated RST on RTD.~
Model_Chains pv_nameplate error
Error results when passing in 'pv_nameplate' input into SystemAnalysis object.

minimum example:
sa = rdtools.SystemAnalysis(df['power_E'], df['poa'], 
                             cell_temperature=df['Tcell'],
                             pvlib_location=loc,
                             pv_tilt=meta['tilt'],
                             pv_azimuth=meta['azimuth'],
                             pv_nameplate = meta['pdc']
                             )#

Error:
---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
<ipython-input-125-55293c7e94e3> in <module>()
----> 1 sa_E.sensor_analysis(analyses=['yoy_degradation'])

c:\users\cdeline\documents\python scripts\kwh_rdtools\rdtools\system_analysis.py in sensor_analysis(self, analyses, yoy_kwargs, srr_kwargs)
    479         '''
    480 
--> 481         self.sensor_preprocess()
    482         sensor_results = {}
    483 

c:\users\cdeline\documents\python scripts\kwh_rdtools\rdtools\system_analysis.py in sensor_preprocess(self)
    438         if self.cell_temperature is None:
    439             self.cell_temperature = self.calc_cell_temperature(self.poa, self.windspeed, self.ambient_temperature)
--> 440         normalized, insolation = self.pvwatts_norm(self.poa, self.cell_temperature)
    441         self.filter(normalized, 'sensor')
    442         aggregated, aggregated_insolation = self.aggregate(normalized[self.sensor_filter], insolation[self.sensor_filter])

c:\users\cdeline\documents\python scripts\kwh_rdtools\rdtools\system_analysis.py in pvwatts_norm(self, poa, cell_temperature)
    257 
    258         pvwatts_kws = {"poa_global": poa,
--> 259                        "P_ref": pv_nameplate,
    260                        "T_cell": cell_temperature,
    261                        "G_ref": 1000,

UnboundLocalError: local variable 'pv_nameplate' referenced before assignment

Solution: has to do with pv_nameplate and self.pv_nameplate naming error.  Will submit pull request momentarily.
Model_Chains pv_nameplate error
Error results when passing in 'pv_nameplate' input into SystemAnalysis object.

minimum example:
sa = rdtools.SystemAnalysis(df['power_E'], df['poa'], 
                             cell_temperature=df['Tcell'],
                             pvlib_location=loc,
                             pv_tilt=meta['tilt'],
                             pv_azimuth=meta['azimuth'],
                             pv_nameplate = meta['pdc']
                             )#

Error:
---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
<ipython-input-125-55293c7e94e3> in <module>()
----> 1 sa_E.sensor_analysis(analyses=['yoy_degradation'])

c:\users\cdeline\documents\python scripts\kwh_rdtools\rdtools\system_analysis.py in sensor_analysis(self, analyses, yoy_kwargs, srr_kwargs)
    479         '''
    480 
--> 481         self.sensor_preprocess()
    482         sensor_results = {}
    483 

c:\users\cdeline\documents\python scripts\kwh_rdtools\rdtools\system_analysis.py in sensor_preprocess(self)
    438         if self.cell_temperature is None:
    439             self.cell_temperature = self.calc_cell_temperature(self.poa, self.windspeed, self.ambient_temperature)
--> 440         normalized, insolation = self.pvwatts_norm(self.poa, self.cell_temperature)
    441         self.filter(normalized, 'sensor')
    442         aggregated, aggregated_insolation = self.aggregate(normalized[self.sensor_filter], insolation[self.sensor_filter])

c:\users\cdeline\documents\python scripts\kwh_rdtools\rdtools\system_analysis.py in pvwatts_norm(self, poa, cell_temperature)
    257 
    258         pvwatts_kws = {"poa_global": poa,
--> 259                        "P_ref": pv_nameplate,
    260                        "T_cell": cell_temperature,
    261                        "G_ref": 1000,

UnboundLocalError: local variable 'pv_nameplate' referenced before assignment

Solution: has to do with pv_nameplate and self.pv_nameplate naming error.  Will submit pull request momentarily.
Update TrendAnalysis notebook to avoid filter index mismatch warning
This adds a few lines to the TrendAnalysis notebook to explicitly reindex the ad_hoc_filter and avoid a warning

- ~Code changes are covered by tests~
- ~New functions added to `__init__.py`~
- ~API.rst is up to date, along with other sphinx docs pages~
- [x] Example notebooks are rerun and differences in results scrutinized
- ~Updated changelog~
Inconsistency in soiling recovery between half_norm_clean and random_clean
**Describe the bug**
When a long invalid interval is made up of a string of shorter invalid intervals, `half_norm_clean` and `random_clean` in `soiling.soiling_srr()` behave differently. `random_clean` allows recovery between invalid intervals.

**Expected behavior**
Both `random_clean` and `half_norm_clean` should treat the entire chain of invalid intervals as a single valid interval without recoveries until the next valid interval is encountered.

**Screenshots**
![Screen Shot 2020-11-10 at 3 43 48 PM](https://user-images.githubusercontent.com/12532260/98742853-5e45b980-236c-11eb-8545-b69847e06349.png)

Zoom in on the `random_clean` case shows unexpected recoveries:
![Screen Shot 2020-11-10 at 3 44 34 PM](https://user-images.githubusercontent.com/12532260/98742861-6140aa00-236c-11eb-9a7b-297fa4630fd9.png)


**Additional context**
Note the example was generated with a partially complete version of  the updates from #199 which changes the behavior to retain invalid intervals at beginning and end of dataset
Soiling SRR behavior change: retain invalid intervals at the beginning and end of the dataset.
The current behavior of the SRR algorithm is to crop off "invalid" intervals at the start and end of the analyzed time frame. Invalid intervals are those that do not qualify to be fit as soiling intervals based on  `min_interval_length` `max_relative_slope_error` and `max_negative_step`. The current behavior is illustrated here:
![image](https://user-images.githubusercontent.com/12532260/115081996-64fbce00-9ec2-11eb-8d51-b677f0fbe6d4.png)

The propsed new behavior is to retain those intervals and apply the assumptions of algorithm. The new behavior, for random clean, would look like this:
![image](https://user-images.githubusercontent.com/12532260/115082048-80ff6f80-9ec2-11eb-9e68-5b74ecf38f5a.png)

clearsky_temperature error when certain lat/lon values passed
There's an uncaught exception in clearsky_temperature.py when certain lat/lon values are passed.  For instance,  
>get_clearsky_tamb(df.index[0:1],35,31)

returns the correct values, but 
>get_clearsky_tamb(times[0:1],35,30)

returns "ValueError: Must have equal len keys and value when setting with an iterable"

The actual issue appears to be when someone inserts an incorrect lat/lon pair that points to ocean instead of land, there's no clearsky temperature to be returned, but the error is not at all descriptive.  This should be an exception that is caught and returned with a message that is somewhat helpful like 'incorrect lat/lon pair - no land found at this point'  or something like that...


clearsky_temperature error when certain lat/lon values passed
There's an uncaught exception in clearsky_temperature.py when certain lat/lon values are passed.  For instance,  
>get_clearsky_tamb(df.index[0:1],35,31)

returns the correct values, but 
>get_clearsky_tamb(times[0:1],35,30)

returns "ValueError: Must have equal len keys and value when setting with an iterable"

The actual issue appears to be when someone inserts an incorrect lat/lon pair that points to ocean instead of land, there's no clearsky temperature to be returned, but the error is not at all descriptive.  This should be an exception that is caught and returned with a message that is somewhat helpful like 'incorrect lat/lon pair - no land found at this point'  or something like that...


Add intersphinx links for pvlib docs where appropriate 
Here's one example from #117 

```suggestion
        Model parameters for pvlib.temperature.sapm_cell(). Used in calculating cell
        temperature from ambient. If string, must be a valid entry
        for sapm model in pvlib.temperature.TEMPERATURE_MODEL_PARAMETERS. If dict, must
        have keys 'a', 'b', 'deltaT'. See pvlib.temperature.sapm_cell() documentation
```

Might be good to set up an [intersphinx link](https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html) to pvlib's docs so that things like this can automatically link to pvlib's API reference.  Future PR.

_Originally posted by @kanderso-nrel in https://github.com/NREL/rdtools/pull/117#discussion_r526980946_
[TrendAnalysis] ad_hoc_filter index isn't checked for consistency with index of other filters
**Describe the bug**
The TrendAnalysis filtering step implicitly assumes that the ad_hoc_filter has the same index as the other filter time series.  In cases where the TrendAnalysis constructor interpolation changes the index of the input time series, which then propagates to the built-in filters, that assumption is violated and the analysis results can be affected.

**To Reproduce**
```python
import pandas as pd
import rdtools

idx1 = pd.date_range('2017-01-01 00:00', '2018-01-01', freq='15min', closed='left')
idx2 = pd.date_range('2018-01-01 00:05', '2019-01-02', freq='15min', closed='left')
times = idx1.append(idx2)

df = pd.DataFrame({
    'poa_global': 1000,
    'power_ac': 1.0,
    'temperature_cell': 25,
}, index=times)

ta = rdtools.analysis_chains.TrendAnalysis(df.power_ac, df.poa_global, df.temperature_cell,
                                           interp_freq='15T')
# disable the quantile clipping filter, it's irrelevant and a nuisance here
ta.filter_params.pop('clip_filter')

# toggle this line on/off:
ta.filter_params['ad_hoc_filter'] = pd.Series(True, df.index)
ta.sensor_analysis()  # fails if above line is uncommented
```

The input time series switches halfway through from being on the minute intervals [0, 15, 30, 45] to [5, 20, 35, 50].  The automatic interpolation realigns the timestamps to all be on [0, 15, 30, 45].  However, the ad_hoc_filter still uses the mixed timestamps.  In the filtering step, this means that some values from the other filters don't have a match in the ad_hoc_filter, so they get set to False.  And likewise the values in the ad_hoc_filter don't have a match in the other filters, so those timestamps get set to False as well.  That means the entire second year gets filtered out, despite the fact that all values in the ad_hoc_filter are True!

**Expected behavior**
Not 100% sure what the correct behavior here should be.  If we're unwilling to try to automatically adjust the ad_hoc_filter to match the post-interpolation time series, which I think is wise, then we should at least alert the user that their ad_hoc_filter probably isn't being used the way they expect.  Some ideas for that alert:

- Check `ad_hoc_filter.index.equals(self.pv_power.index)`, or something like that, and raise an error if the indexes aren't equal.  The way to prevent such an error would be for the user to create the TrendAnalysis instance, then calculate their ad_hoc_filter using the interpolated TrendAnalysis attributes so that the indexes are the same when it's time to filter.
- If the above option turns out to be too strict (for example maybe the interpolation creates some very minor difference like an extra nighttime value because of a DST transition), another option is to require all points that pass the other filters to have matches in the ad_hoc_filter, or in Mike's words "everything that stays in at the end of the day has to be explicitly represented in the ad hoc filter".  That would allow some flexibility around nuisance issues for values that don't matter, but still be strict on the values that do. 

**Additional context**
#263 might be relevant



Discussion / hints:

@mdeceglie @cdeline I think this is ready for review.  It's pretty huge though so I'd guess that it'd be easier to talk through everything in person.  Let me know what you think.  
make HTML is working for me.  The docs look really good.
I'm getting an import error for statsmodels when importing from requirements.txt.  This is a known issue that appears to be fixed in 0.10.1 of statsmodels (?)  https://github.com/statsmodels/statsmodels/issues/4635
Alright, I got TravisCI happy again, but readthedocs.io is still timing out on the build.  https://readthedocs.org/projects/rdtools
At the moment I've been focusing on getting things building with requirements.txt, but the suggestion is to set up a separate requirements textfile for building documentation.  It can obviously be more streamlined to set up documentation rather than actually running RdTools.
Details on the resources available for buildthedocs are here. Other suggestions on reducing build time is on page 58 https://buildmedia.readthedocs.org/media/pdf/docs/latest/docs.pdf#page=34&zoom=100,0,584
I don't understand that output of the RTD builds -- it says the total time was 15 minutes, but the commands listed only total to a handful of seconds.  Maybe the issue is something like the job isn't configured to actually build the documentation, so the job never completes? 
I believe it - trying to install all of those packages from a bare environment can take some time.  There's an option to use pre-installed packages on the server with a config.yml file.  PVlib takes that approach - I'll copy their config file and see if that helps anything.  Also, I'll set up a secon requirements.txt file just for the documentation - we can probably streamline the required packages a tad.
"master doc" failures might be because we aren't specifying it in `conf.py` -- see https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-master_doc

`index.rst` contains our root toctree, so I suppose it should be set to `'index'`.
Docs passing!
https://readthedocs.org/projects/rdtools/builds/9952713/
Very cool :) Only issue I see is that the screenshots aren't showing up on the landing page.  The problem is that the translation of README.md from markdown to RST doesn't update the relative paths to those images, so they don't work in the built HTML.  The build script I was using had a copy command at the end to bring the screenshots to the right place, but the makefile doesn't currently have that.  It might be possible to rewrite the paths in the markdown file so that they work in both the README on github and after translation on RTD, or maybe the build process should just copy them over like I was doing.  
I hadn't noticed before, but the same issue (relative paths not getting translated) broke the links to example notebooks and such as well.  Maybe an easy fix for those links would just be to change them to point at RTD now that it exists? 
Kevin - if you create a username on ReadTheDocs, I can add you as an administrator.  Right now it says kanderso-nrel does not exist when I try to add you as an administrator

Done, same account name.  
Kevin - I was able to fix this by adding a symbolic link to sphinx/build/html back to /screenshots.  You don't need to change anything in readme.md
I was worried that a symlink wouldn't play nicely with Windows dev environments, but if it works then that's fine.  The latest commit (renaming the screenshots folder to _static and changing doc links from Github to RTD) appears to work as well.  
OK, let's do it your way.  The symlinks are working weird with git.
Closed by #133? 
Yes, PR #133 fixes it in model_chains
Closed by #133? 
Yes, PR #133 fixes it in model_chains








Fix patch:
diff --git a/.github/workflows/requirements.yaml b/.github/workflows/requirements.yaml
index 690500dc..4bf39728 100644
--- a/.github/workflows/requirements.yaml
+++ b/.github/workflows/requirements.yaml
@@ -1,6 +1,12 @@
 name: requirements
 
-on: [pull_request, push]
+on:
+  push:
+    branches:
+    - master
+    - development
+  pull_request:
+
 
 jobs:
   requirements:
diff --git a/docs/sphinx/source/changelog/pending.rst b/docs/sphinx/source/changelog/pending.rst
index f3f4f226..95e46e5c 100644
--- a/docs/sphinx/source/changelog/pending.rst
+++ b/docs/sphinx/source/changelog/pending.rst
@@ -11,3 +11,5 @@ Requirements
 * Bump ``ipython==7.16.3``, ``jupyter-console==6.4.0``,
   and ``prompt-toolkit==3.0.27`` in ``docs/notebook_requirements.txt``
   and bump ``Pillow==9.0.0`` in ``requirements.txt`` (:pull:`314`)
+* Bump ``nbsphinx`` version from 0.8.5 to 0.8.8 in the optional
+  ``[doc]`` requirements (:pull:`317`)
diff --git a/setup.py b/setup.py
index 5eb8cbff..add256aa 100644
--- a/setup.py
+++ b/setup.py
@@ -56,7 +56,7 @@
 EXTRAS_REQUIRE = {
     'doc': [
         'sphinx==3.2',
-        'nbsphinx==0.8.5',
+        'nbsphinx==0.8.8',
         'nbsphinx-link==1.3.0',
         'sphinx_rtd_theme==0.5.2',
         'ipython',


Test patch:
diff --git a/.github/workflows/pytest.yaml b/.github/workflows/pytest.yaml
index 70c869e1..393a5d43 100644
--- a/.github/workflows/pytest.yaml
+++ b/.github/workflows/pytest.yaml
@@ -1,6 +1,12 @@
 name: pytest
 
-on: [pull_request, push]
+on:
+  push:
+    branches:
+    - master
+    - development
+  pull_request:
+
 
 jobs:
   build: