Skip to content

Update tutorial notebooks for compat with current pvlib #999

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

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Jul 8, 2020

  • Closes Update tutorials for pvlib >=0.7 #995
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Here is an updated set of notebooks. I'll echo @mikofski's suggestion to use nbdime to review the changes. If you clone this branch, you can spawn a handy visual diff between commits in the browser with nbdiff-web 065e10c 97c6698 docs\tutorials\atmosphere.ipynb

I also made some other changes unrelated to pvlib compat -- semicolons to suppress matplotlib output text and such. I also removed the section in tmy.ipynb that called out to the RREDC website (see #996). Lastly, I made the notebooks work with the nbval pytest plugin (added comments like # NBVAL_SKIP for cells that shouldn't be tested) -- run pytest --nbval docs\tutorials\atmosphere.ipynb if you want to check it out.

I am aware of two remaining issues, neither of which I have investigated yet. I would not be upset if someone else figures out what's going on with these before I do :)

  • pvsystem.ipynb: de soto modeling at the end is different
  • tracking.ipynb: some traces get pretty wacky in the negative axis tilt plots (north-facing)

@cwhanse
Copy link
Member

cwhanse commented Jul 9, 2020

@kanderso-nrel about pvsystem.ipynb: when I vew the notebook via the "Files changed" tab, all looks fine. When I view the notebook via the link on the RTD index page, there are Type errors in the desoto section, and a strange message in the Single diode model section.

Maybe that provides some clues?

@kandersolar
Copy link
Member Author

I think the link on the RTD index page is pointing to the current version on master, which I agree has some errors. I was trying to get the notebooks in this PR to recreate the versions on master, but I'm starting to think that the version of pvsystem.ipynb on master is not self-consistent, at least in the De Soto section and after. Putting aside the error, some results seem strange (Rsh<20?). So if this PR's version of pvsystem.ipynb looks acceptable on its own, I'm inclined to call it good.

For tracking.ipynb, maybe we should leave it with some funky plots until we pick up #823 again? I suspect that implementing the method Mark and I worked out for non-parallel slope would solve those problems as a side effect.

@cwhanse
Copy link
Member

cwhanse commented Jul 15, 2020

Putting aside the error, some results seem strange (Rsh<20?).

The CEC module parameters changed between the two notebooks. SAM used to supply a generic "Example Module", maybe it still does, but we're no longer using that module in the notebook or test fixtures, which I think is an improvement.

So if this PR's version of pvsystem.ipynb looks acceptable on its own, I'm inclined to call it good.

Agree. There's a lot of little edits I'd suggest but I don't know how to comment on the ipynb file, so OK for now.

For tracking.ipynb, maybe we should leave it with some funky plots until we pick up #823 again? I suspect that implementing the method Mark and I worked out for non-parallel slope would solve those problems as a side effect.

Also agree.

I've looked at the new notebooks. I agree that the PR meets its objective, that the notebooks work with pvlbi v0.7.2. Further improvement of the notebooks can be in different PRs.

@kanderso-nrel what do you need next here?

@cwhanse cwhanse added this to the 0.8.0 milestone Jul 15, 2020
@kandersolar
Copy link
Member Author

Thanks @cwhanse for going through the notebooks. I don't intend to make any more changes to this PR unless requested from reviewers.

I do still think it's worth trying to add the notebooks to the CI testing using nbval, but figuring out the best way might take some iteration and I'd prefer to do that in another PR later. For now the notebooks can be tested with nbval locally if anyone cares to, which is a good step forward itself in addition to bringing the notebooks up to date with the current API.

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

let's take #329 more seriously before we think about improving tooling around notebooks.

@cwhanse cwhanse merged commit ca5d62f into pvlib:master Jul 15, 2020
@kandersolar kandersolar deleted the tutorials_update branch July 15, 2020 18:52
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.

Update tutorials for pvlib >=0.7
3 participants