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

Update master for release #73

Merged
merged 44 commits into from
Mar 30, 2018
Merged

Update master for release #73

merged 44 commits into from
Mar 30, 2018

Conversation

mdeceglie
Copy link
Collaborator

This incorporates changes that address issues: #67, #61, #59, #57, #56 as well as:

  • improvements/typo fixes to docstrings
  • Fixes error in check for two years of data in degradation_year_on_year
  • Improves the calculations underlying irradiance_rescale

All changes have ben through review when incorporated into the development branch. The purpose of this PR is to check that this version of rdtools installs and runs correctly.

For me, it passes everything in the release procedure, with the exception of the minimum requirement check on windows, but failure was due to problems building old versions of some requirements.

Michael Deceglie and others added 30 commits December 15, 2017 13:23
Update the normalized_energy info in degradation_year_on_year
…es from production-weather merged files (csv or pickle)
…es from production-weather merged files (csv or pickle)
…tion rates from production-weather merged files (csv or pickle)"

This reverts commit 09fbb33.
…tion rates from production-weather merged files (csv or pickle)"

This reverts commit 47c996e.
Merge issue_57 branch with development branch
Enables users to specify the size of the returned confidence interval
Fix erroneous check for two years of data
Some changes in the notebook were overwritten by update from
development branch. Also explicitly set method = ‘iterative” in
rdtools.irradiance_rescale()
Michael Deceglie and others added 13 commits February 13, 2018 21:12
… nosie

Previous version used approximations such as 52 weeks in a year. This
version uses 1 year = 365 days regardless of the frequency. It also
adds noise so that the confidence interval calculations can be tested.
Finally I simplified the structure.
* closes #56
* running `2to3 rdtools/` shows the only incompatibilities are the
 relative imports already addressed in #67 so merge #68 to fix imports
* running `2to3 tests/` shows python 2 print commands, so rerun 2to3
with `-f print -w -n` to add parentheses to print statements to make
them calls instead of commands, altho these should really be replaced
by logging, so TODO: make another issue and PR to add logging to tests

Signed-off-by: Mark Mikofski <[email protected]>
Copy link
Collaborator

@abshinn abshinn left a comment

Choose a reason for hiding this comment

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

Hi @mdeceglie. Thanks for putting this release together.

I ran python setup.py install in clean environments for both python 2.7 and python 3.6 using the latest development branch. Both installed without error.

When running tests/run_tests, in both python versions, all tests pass but I received the familiar FutureWarnings about pandas-statsmodels. Issue is already noted in #27. Not a show stopper for this release.

@cdeline
Copy link
Collaborator

cdeline commented Mar 30, 2018

I get installation errors in new environments of Python 2.7 and Python 3.6 with my Win10 machine. When installed into my existing environment, there are no errors.

Python 2.7:
from numpy.distutils.misc_util import get_info
ModuleNotFoundError: No module named 'numpy'

Python 3.6:
from numpy.distutils.misc_util import get_info
ImportError: No module named numpy.distutils.misc_util

This appears to be a known bug with statsmodels for Windows 10 machines:

statsmodels/statsmodels#3207

https://stackoverflow.com/questions/45492810/statsmodels-installation-no-module-named-numpy-distutils-msvccompiler-in-num

image

image

@cdeline
Copy link
Collaborator

cdeline commented Mar 30, 2018

Update: if I use 'conda install statsmodels' followed by 'pip install .' to install the development branch, then everything works in py2.7 and py3.6. Is there any way to install statsmodels through conda rather than pip in our setup.py file?

@mdeceglie
Copy link
Collaborator Author

@cdeline I was able to recreate this in a clean environment with only pip and python. However, I suggest we can proceed without directly addressing this. If a user has installed the anaconda python distribution (that's the most likely way they would get the conda package manager), they will already have statsmodels installed. If they are setting up environments, hopefully they have enough experience to navigate the requirements gymnastics.

We could add a note in the readme encouraging people to install requirements by hand if they run into trouble. Do you think that would help?

@cdeline
Copy link
Collaborator

cdeline commented Mar 30, 2018

OK, I'm good with this approach. If you could include an extra line in the readme about manually installing components using conda if they get errors during install, I'm fine with this.
I suspect that we'd still have installation issues if someone has a fresh non-Anaconda python distribution and just tries 'pip install rdtools', We don't specifically require Anaconda in the readme anywhere.

Copy link
Collaborator

@cdeline cdeline left a comment

Choose a reason for hiding this comment

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

Update readme to include a line about manually installing components using conda if necessary. Otherwise, everything installs and runs correctly.

@mdeceglie
Copy link
Collaborator Author

Agreed @cdeline, how's the updated readme look?

@mdeceglie mdeceglie merged commit af466d1 into master Mar 30, 2018
@mdeceglie
Copy link
Collaborator Author

Thanks for the input everyone!

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.

5 participants