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

Mount gallery examples #1266

Merged
merged 8 commits into from
Aug 7, 2021
Merged

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Jul 30, 2021

  • Closes #xxxx
  • 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.

Rendered examples:

@kandersolar kandersolar added this to the 0.9.0 milestone Jul 30, 2021
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.

Thanks @kanderso-nrel for the great examples.

The Example Gallery in this PR is feeling a bit disorganized. Have we previously discussed adding subsections to the Example Gallery? I feel like we have but I don't remember where, when, or anything that might have been said. Certainly not required in this PR.

# Let's take a look at the tracker rotation curve it produces:

# a simple weather dataset for illustration -- one day of 1-minute weather
DATA_DIR = pathlib.Path(pvlib.__file__).parent / 'data'
Copy link
Member

Choose a reason for hiding this comment

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

Is the downward spike due to DNI momentarily dropping to 0 in the weather data file? While it might be nice to highlight the read_bsrn function here, the spike is the most noticeable thing in the results. My concern is that the people that would most benefit from this example are also the people most likely to be distracted by that spike.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that spike either, but that was the only clear-sky day in the file. Would it be better to keep it simple and do a quick clear-sky simulation instead of using the BSRN data?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say yes but it's not a clear win and I certainly don't insist on it.

system = pvsystem.PVSystem(arrays=[array], inverter_parameters={'pdc0': 1})
mc = modelchain.ModelChain(system, loc, spectral_model='no_loss')

_ = mc.run_model(weather)
Copy link
Member

Choose a reason for hiding this comment

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

Is the underscore to suppress output? I'm not sure about sphinx-gallery, but the ipython directive will suppress output if you end the line in ;. No big deal to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, to suppress output. Semicolon doesn't suppress in sphinx-gallery, at least in my local build:

image

Copy link
Member Author

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Have we previously discussed adding subsections to the Example Gallery?

Briefly, see the conversation starting here: #1077 (comment). I think if we agree on what the sections should be, it should be an easy change to make.

# Let's take a look at the tracker rotation curve it produces:

# a simple weather dataset for illustration -- one day of 1-minute weather
DATA_DIR = pathlib.Path(pvlib.__file__).parent / 'data'
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that spike either, but that was the only clear-sky day in the file. Would it be better to keep it simple and do a quick clear-sky simulation instead of using the BSRN data?

system = pvsystem.PVSystem(arrays=[array], inverter_parameters={'pdc0': 1})
mc = modelchain.ModelChain(system, loc, spectral_model='no_loss')

_ = mc.run_model(weather)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, to suppress output. Semicolon doesn't suppress in sphinx-gallery, at least in my local build:

image

@wholmgren
Copy link
Member

Let's defer example organization for now. I originally wanted to organize by object layer vs not object layer but perhaps other approaches (e.g. how do I do this?) are better. And I don't want to put the thought into it now.

@wholmgren
Copy link
Member

thanks @kanderso-nrel!

@wholmgren wholmgren merged commit 0e6fea6 into pvlib:master Aug 7, 2021
@kandersolar kandersolar deleted the mount_gallery_examples branch August 7, 2021 16:15
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.

None yet

2 participants