Skip to content

Fix crash in ModelChain.infer_spectral_model due to incorrect method call #620

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.

Sign up for GitHub

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

Jump to bottom
Closed
jgaffiot wants to merge 9 commits into from
Closed

Fix crash in ModelChain.infer_spectral_model due to incorrect method call #620

jgaffiot wants to merge 9 commits into from

Conversation

@jgaffiot
Copy link

@jgaffiot jgaffiot commented Nov 20, 2018 •
edited
Loading

  • Closes issue modelchain: crash in ModelChain.infer_spectral_model due to incorrect method call #619
  • I am familiar with the contributing guidelines.
  • [NO] I got numerous crashes of test, with all the same message: mocker object missing. Also no test of the method infer_spectral_model exist in test_modelchain.
  • [na] Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • [na] New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

cwhanse
cwhanse reviewed Nov 20, 2018
View reviewed changes
docs/sphinx/source/whatsnew/v0.6.1.rst Outdated
and < 1900, fix year 2050 which was returning 0.
* Fix and improve :func:`~pvlib.solarposition.hour_angle` (:issue:`598`)
* Fix error in :func:`pvlib.clearsky.detect_clearsky` (:issue:`506`)
* Fix error in :func:`pvlib.modelchain.ModelChain.infer_spectral_model` (:issue:`507`)
Copy link
Member

@cwhanse cwhanse Nov 20, 2018

Choose a reason for hiding this comment

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

Issue number is incorrect here, should be #619

Copy link
Member

@cwhanse cwhanse Nov 20, 2018

Choose a reason for hiding this comment

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

Thanks for the bug report and the PR!

It looks like this current PR builds on some work from 2017, which may explain the test failures. Could you redo the PR, first pull the current master and then apply the change to ModelChain?

Copy link
Author

@jgaffiot jgaffiot Nov 20, 2018

Choose a reason for hiding this comment

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

That's what (I think) I did, fetching the upstream as explained in the github doc. It's just my fork is old, but clearly the modifications are only a few lines.
When running the test locally, it appears that I have pytest v4.0.0, brand new, it may explain the problem.
Otherwise, I have to re-fork the project ?

Copy link
Member

@wholmgren wholmgren Nov 20, 2018

Choose a reason for hiding this comment

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

see #623 for existing test failures. this pr will need a new test that would fail without your fix.

@cwhanse cwhanse mentioned this pull request Nov 20, 2018
Closed
@wholmgren wholmgren added the bug label Nov 21, 2018
@wholmgren wholmgren added this to the 0.6.1 milestone Nov 21, 2018
@jgaffiot jgaffiot mentioned this pull request Dec 7, 2018
Merged
6 tasks
@jgaffiot
Copy link
Author

jgaffiot commented Dec 7, 2018

Replaced by clean PR #629

@jgaffiot jgaffiot closed this Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wholmgren wholmgren wholmgren left review comments

@cwhanse cwhanse cwhanse left review comments

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

0.6.1

Development

Successfully merging this pull request may close these issues.

3 participants

@jgaffiot @wholmgren @cwhanse