-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Projected zenith convenience function #1904
Projected zenith convenience function #1904
Conversation
Such a diagram might be helpful, but no need in this PR IMHO. I'm not sure where it would live anyway. I don't think we currently have any diagrams in function-level documentation. I guess it could be in a gallery example (or a new User's Guide page) but unless it improves on the references, I'd say let's not bother :)
Is there a reason this function needs JIT more than the rest of pvlib does? We use
Slope-Aware Backtracking has a small validation dataset in a table at the end. It's not enough on its own for this PR, but it's at least a start, and can be easily extended in at least one way (subtract 180 from |
I can't make this work. Could you double check if that's supposed to happen? # test by changing array azimuth
psz = psz_func(
array_tilt,
array_azimuth-180,
timedata["Apparent Elevation"],
timedata["Solar Azimuth"],
)
assert_allclose(psz, -timedata["True-Tracking"], atol=1e-3) |
My mistake, I forgot those test cases used a nonzero |
I've left out whether the result is valid like |
Reported at: pvlib#1725 (comment) Confirmed via "Slope-Aware Backtracking for Single-Axis Trackers", paragraph after Eq. 1 Co-Authored-By: Mark Mikofski <bwana.marko@yahoo.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial notes
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
@kandersolar , GitHub does not allow me to reply to your last review message, so here it goes:
I agree it's a good point. It's difficult to understand exactly the vectors and the projection plane without reading it. In fact, such a recommendation would be a nice addition to the notes section.
I also agree with that. But regarding the suggested explanations, I think it would be better to describe the use cases with real examples in general. If I didn't already know about the internals and purposes, I wouldn't understand why
I wouldn't change the current arguments definitions (almost due to the 1:1 relation with the paper). Maybe move your suggested clarifications to the notes section, although I'm not sure of how to integrate them. I'd like to clear up that I don't have a problem applying your suggestions straight forward if you want to, specially due to my lack of experience in PV and the language barrier. And that I hope this comment helps improving the quality of the PR and doesn't add noise to the conversation. |
I'd like to move forward this PR another bit, specially the docstring. Would you like me to push @kandersolar suggestions, or give another suggestions? Feel free to request anything from my side. |
Thanks @echedey-ls for keeping this going :) Describing some real use cases is a great idea, and I like your proposed text. What do you think about a combination of
|
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
I like that pretty much, thanks for the improvements. I will add the still inexistent You can check the docs out here. |
Fixed the link finally
I had some trouble finding the example's link. I can add a section link too but reading it all seems a better choice. From my side, this looks like it can be merged! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One trivial suggestion so that the image doesn't take up so much webpage space, but otherwise LGTM!
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
Thanks for this contribution and the patience @echedey-ls :) |
docs/sphinx/source/reference
for API changes.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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Adds a function that calculates the projected solar zenith.
Pending tasks:
[ ] Add diagram?singleaxis
, change it's implementation to useprojected_solar_zenith_angle
and change tests accordingly.Test data generation script for the PSZ implementation port from `singleaxis` to `projected_solar_zenith_angle`
Documentation links
Link to function.