Skip to content

Add atand function #1927

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 6 commits into from
Dec 12, 2023
Merged

Add atand function #1927

merged 6 commits into from
Dec 12, 2023

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Dec 6, 2023

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

pvlib has convenience functions for calculating sine, cosine, and tangent in degrees. Similarly, it has functions for arcsine and arccosine but NOT arctangent, which seems inconsistent.

I couldn't find anywhere these functions are tested, but maybe they just aren't?

@kandersolar kandersolar added this to the v0.10.3 milestone Dec 11, 2023
Co-authored-by: Kevin Anderson <[email protected]>
Thanks for these improvements @adriesse

Co-authored-by: Anton Driesse <[email protected]>
@AdamRJensen
Copy link
Member Author

I will say that I wish we would have named inverse sine arcsind and not asind to stay consistent with the Numpy naming which calls the radian equivalent (which we wrap) for np.arcsin. @kandersolar do you know if this was an intentional choice?

@echedey-ls
Copy link
Contributor

Would atan2d be a good addition to this PR too? Or arctan2, following last comment.

@AdamRJensen
Copy link
Member Author

Would atan2d be a good addition to this PR too? Or arctan2, following last comment.

Certainly not a bad idea - I think I'll hold off adding it until I need it though :P

@kandersolar
Copy link
Member

asind is among the oldest bits of pvlib python: e74644b#diff-0122268ff3ca7c61f9b9be534e547dbdfff3a7509577e704e456f06091908dfbR250-R252

I presume the choice of name was motivated by convenience when translating to python from MATLAB (which also uses asind).

@AdamRJensen AdamRJensen mentioned this pull request Dec 11, 2023
15 tasks
@echedey-ls
Copy link
Contributor

Would atan2d be a good addition to this PR too? Or arctan2, following last comment.

Certainly not a bad idea - I think I'll hold off adding it until I need it though :P

For future reference, search of np.degrees(np.arctan2( among all pvlib module raises 16 results.

Valid point anyway.

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.

Since normally I probably wouldn't be in favor of adding functions to pvlib.tools without it being used somewhere in pvlib, I'll point out that we expect to use atand in an upcoming PR.

Thanks @AdamRJensen

@kandersolar kandersolar merged commit 6ff02e5 into pvlib:main Dec 12, 2023
@AdamRJensen AdamRJensen deleted the add_atand branch December 12, 2023 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants