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.

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

add spectrum.average_photon_energy gallery example #2206

Merged
merged 23 commits into from
Sep 24, 2024
Merged

add spectrum.average_photon_energy gallery example #2206

merged 23 commits into from
Sep 24, 2024

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Sep 11, 2024 •
edited
Loading

  • Closes Example gallery suggestion: spectrum.average_photon_energy #2194
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference 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 (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

This example demonstrates the use of spectrum.average_photon_energy with the output of spectrum.spectrl2 as an input.

Link to example here

@RDaxini RDaxini mentioned this pull request Sep 11, 2024
Closed
@echedey-ls
Copy link
Contributor

For everyone else stalking Dax progress with each commit: https://pvlib-python--2206.org.readthedocs.build/en/2206/gallery/spectrum/average_photon_energy.html#sphx-glr-gallery-spectrum-average-photon-energy-py

@RDaxini RDaxini marked this pull request as ready for review September 18, 2024 15:27
@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 18, 2024

Tentatively marking this ready for review. Would be good to get some feedback at this stage.

@RDaxini RDaxini changed the title [WIP] add spectrum.average_photon_energy gallery example add spectrum.average_photon_energy gallery example Sep 18, 2024
IoannisSifnaios
IoannisSifnaios reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@IoannisSifnaios IoannisSifnaios left a comment

Choose a reason for hiding this comment

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

Looks good @RDaxini! Some minor edits and ideas from my side.

docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
@kandersolar kandersolar added this to the v0.11.1 milestone Sep 23, 2024
@kandersolar kandersolar mentioned this pull request Sep 23, 2024
Closed
11 tasks
@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 23, 2024 •
edited
Loading

Thanks for the review @IoannisSifnaios
I have now resolved the issues commented so far

kandersolar
kandersolar reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@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.

I like it. It's a bit lengthy for a gallery example, but until we have a better place for long-form tutorial content, I think this is fine.

A few suggestions below.

docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
RDaxini and others added 3 commits September 24, 2024 13:28
@RDaxini @kandersolar
Apply suggestions from code review
5d6f7e6 
Co-authored-by: Kevin Anderson <[email protected]>
@RDaxini
Update average_photon_energy.py
b2164d7 
add =
variable names
remove unused package
@RDaxini
Update average_photon_energy.py
8bfc2cf 
gallery example ref
@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 24, 2024

I have resolved all outstanding issues. Let me know if there's anything else

It's a bit lengthy for a gallery example, but until we have a better place for long-form tutorial content, I think this is fine.

^hmm I had been thinking along similar lines

kandersolar
kandersolar approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@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.

LGTM!

docs/sphinx/source/whatsnew/v0.11.1.rst Outdated Show resolved Hide resolved
echedey-ls
echedey-ls approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@echedey-ls echedey-ls left a comment

Choose a reason for hiding this comment

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

LGTM too.

Minor visualization suggestions down below; they ain't blockers.

docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
docs/examples/spectrum/average_photon_energy.py Outdated Show resolved Hide resolved
RDaxini and others added 2 commits September 24, 2024 14:09
@RDaxini @kandersolar
Update docs/sphinx/source/whatsnew/v0.11.1.rst
7687ddb 
Co-authored-by: Kevin Anderson <[email protected]>
@RDaxini @kandersolar @echedey-ls
Update average_photon_energy.py
5cfc22d 
update legend

Co-Authored-By: Kevin Anderson <[email protected]>
Co-Authored-By: Echedey Luis <[email protected]>
@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 24, 2024

@echedey-ls @kandersolar see the last commit, does that look better? Let me know what you think. Happy to revise further if necessary. I appreciate the review/suggestions

@echedey-ls
Copy link
Contributor

Yeah! It super good.

@kandersolar kandersolar merged commit cc700f9 into pvlib:main Sep 24, 2024
24 of 25 checks passed
@kandersolar
Copy link
Member

Thanks @RDaxini :)

@RDaxini RDaxini deleted the ape_example branch September 24, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IoannisSifnaios IoannisSifnaios IoannisSifnaios left review comments

@echedey-ls echedey-ls echedey-ls approved these changes

@kandersolar kandersolar kandersolar approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
v0.11.1
Development

Successfully merging this pull request may close these issues.

Example gallery suggestion: spectrum.average_photon_energy
4 participants
@RDaxini @echedey-ls @kandersolar @IoannisSifnaios