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

aoi definition in variables_style_rules.csv #2247

Merged
merged 3 commits into from
Oct 18, 2024
Merged

aoi definition in variables_style_rules.csv #2247

merged 3 commits into from
Oct 18, 2024

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Oct 9, 2024

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

Original definition says angles "between 90deg and 90deg", but this should be plus/minus 90deg (right?)

@cwhanse
Copy link
Member

cwhanse commented Oct 9, 2024

I read "angle...between" as going to tell me the two rays that define the angle, rather than the limits. What about "angle between the normal vector and the vector pointing to the sun's center"? I also think we can drop the limits +/-90 since pvlib doesn't enforce that.

@echedey-ls
Copy link
Contributor

In addition to @cwhanse note, I would add normal vector of the module. To be very clear.

@kandersolar
Copy link
Member

Just a general comment: AOI goes from 0 to 180. Negative AOI doesn't make sense.

@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 9, 2024 •
edited
Loading

Just a general comment: AOI goes from 0 to 180. Negative AOI doesn't make sense.

I was wondering about that...

I was trying to make sense of this line:

pvlib-python/pvlib/iam.py

Line 273 in cbe4cc5

-90 <= aoi <= 90, therefore `iam` is constrained to 0.0 outside this

Maybe in this case, the normal to the surface is defined as the 0, and then +/- 90 either side? (not saying that's a correct way to do it, just trying to interpret what was written originally)
As far as I can tell the only iam=0 is set in line 228 for aoi>=90 (no mention of any negatives) so does this function docstring need revising?

Anyway, I'll remove the range reference as suggested by @cwhanse

@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 9, 2024 •
edited
Loading

I read "angle...between" as going to tell me the two rays that define the angle, rather than the limits.

Makes sense

"the vector pointing to the sun's center"

Would "sun's rays" be sufficient here or is it not precise enough? (I looked here on sciencedirect and here on sandia for lexical inspiration 👀)

@cwhanse
Copy link
Member

cwhanse commented Oct 9, 2024 •
edited
Loading

@RDaxini that text probably originated in PVLib for Matlab, as a warning to the user when AOI<0 was input. When AOI<0, the Martin-Ruiz function would give IAM<0. At some point in transfer to pvlib-python or creating of iam.py from functions in pvsystem.py, the message was confused.

iam.martin_ruiz handles aoi<0 by setting the IAM to zero.

To be clear, I am suggesting deleting the range from the description of AOI in the Glossary. I would correct, not delete, the statement in the martin_ruiz docstring that IAM= for AOI outside of 0 <= aoi <=90.

Let's not mix editing docstrings with creating a glossary in this PR, it won't ever get finished.

It would be very helpful if you can create an issue to accumulate docstring edits as a checklist. An entry in that checklist: this line is a cut/paste error and should be removed.

@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 9, 2024

To be clear, I am suggesting deleting the range from the description of AOI in the Glossary. I would correct, not delete, the statement in the martin_ruiz docstring that IAM= for AOI outside of 0 <= aoi <=90.

Understood, we are on the same page here

Let's not mix editing docstrings with creating a glossary in this PR, it won't ever get finished.

Agreed

It would be very helpful if you can create an issue to accumulate docstring edits as a checklist. An entry in that checklist: this line is a cut/paste error and should be removed.

Okay I will do this 👍🏾

@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 9, 2024 •
edited by cwhanse
Loading

this line is a cut/paste error and should be removed.

@cwhanse is the same line on 381 and 471 correct?

Yes, but it belongs in those other two functions martin_ruiz_diffuse and interp

@RDaxini RDaxini mentioned this pull request Oct 9, 2024
Closed
2 tasks
@RDaxini @cwhanse @echedey-ls
update definition
62fd300 
Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>
Co-Authored-By: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@RDaxini @kandersolar
Update pvlib/data/variables_style_rules.csv
629bff4 
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 15, 2024

"the vector pointing to the sun's center"

Would "sun's rays" be sufficient here or is it not precise enough? (I looked here on sciencedirect and here on sandia for lexical inspiration 👀)

Any thoughts on this? Just an idea, not a definite suggestion.

@kandersolar kandersolar added this to the v0.11.2 milestone Oct 18, 2024
@kandersolar
Copy link
Member

The sun's rays originate from a disc with an angular diameter of a degree or so. It is the center of this disc which defines aoi, in the sense that pvlib uses it.

Seems like nobody objects to the definition as it currently stands in this PR, so I'll go ahead and merge it. Thanks @RDaxini!

@kandersolar kandersolar merged commit 1c65dca into pvlib:main Oct 18, 2024
27 checks passed
@RDaxini RDaxini deleted the aoi-definition branch October 18, 2024 16:50
RDaxini added a commit to RDaxini/pvlib-python that referenced this pull request Oct 19, 2024
@RDaxini
updated aoi definition pvlib#2247
816f43d
kandersolar added a commit that referenced this pull request Oct 31, 2024
@RDaxini @AdamRJensen
@echedey-ls @kandersolar @cwhanse
Change Variables and Symbols page into a Glossary of terms (#2234)
0781787 
* initial test

* Update irradiance.py

* update list of terms

* update dni description in irradiance.py

* minor edits, aoi definition

* Update glossary.rst

* Update glossary.rst

* remove in-text dni term glossary link

* delete one surface_tilt definition

* review: dni definition

Co-Authored-By: Adam R. Jensen <39184289+adamrjensen@users.noreply.github.com>

* remove old files, add general text to glossary

* remove first bullet, add colon, on SoDa line

* Update glossary.rst

* conciseness (suggestion)

* updated aoi definition #2247

* update references old variables style rules page

* remove old page link from v0.3.0 whatsnew

* Update index.rst

Co-Authored-By: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>

* Update v0.11.2.rst

* Update docs/sphinx/source/user_guide/glossary.rst

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

* Update docs/sphinx/source/contributing/style_guide.rst

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

* add underscore

* changed reference label

* update labels

* name: nomenclature, update labels

* update whatsnew wording

* update file name

---------

Co-authored-by: Adam R. Jensen <39184289+adamrjensen@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kandersolar kandersolar kandersolar left review comments

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

Successfully merging this pull request may close these issues.
4 participants
@RDaxini @cwhanse @echedey-ls @kandersolar