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

Implement reverse transposition using Perez-Driesse forward transposition #1907

Merged
merged 28 commits into from
Dec 12, 2023

Conversation

adriesse
Copy link
Member

@adriesse adriesse commented Nov 12, 2023

@adriesse adriesse marked this pull request as draft November 12, 2023 20:46
@adriesse
Copy link
Member Author

The function works, for anyone who wants to play with it already. The discussion about suitable function names is now open!

@adriesse adriesse marked this pull request as ready for review November 14, 2023 15:10
@kandersolar kandersolar added this to the v0.10.3 milestone Nov 15, 2023
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.

Nice PR! Here are a few quick comments.

pvlib/irradiance.py Show resolved Hide resolved
pvlib/irradiance.py Show resolved Hide resolved
@adriesse
Copy link
Member Author

@pvlib/pvlib-maintainer it is unclear whether potential reviewers are actually aware that this PR is ready for review. Is there any way to tell?

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Thanks @adriesse looks pretty good to me.

pvlib/irradiance.py Outdated Show resolved Hide resolved
pvlib/irradiance.py Outdated Show resolved Hide resolved
pvlib/irradiance.py Outdated Show resolved Hide resolved
@adriesse
Copy link
Member Author

@AdamRJensen Do you think we will have the final form of the paper reference in time for the December release?

@AdamRJensen
Copy link
Member

@AdamRJensen Do you think we will have the final form of the paper reference in time for the December release?

I'm a bit puzzled that it is not out yet, but I expect it soon. At the very least we can add the doi: https://doi.org/10.1016/j.solener.2023.112093

@adriesse
Copy link
Member Author

So to wrap this up I would:

  • do a global change of "rtranspose" to "ghi_from_poa"
  • add the DOI to the references

Anything else needed?

@adriesse
Copy link
Member Author

adriesse commented Dec 2, 2023

Wow, do I get a badge for making changes that do not trigger Flake8 ??

@adriesse
Copy link
Member Author

adriesse commented Dec 2, 2023

Ready to merge?

@adriesse
Copy link
Member Author

adriesse commented Dec 3, 2023

I guess I'll wait a bit in case more reviews trickle in. It ain't over 'til it's over.

@AdamRJensen
Copy link
Member

For the reverse transposition example it would be great to incorporate the GTI_DIRINT function, if anything, at least mention that pvlib offers two reverse transposition algorithms.

…ons.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
@adriesse
Copy link
Member Author

adriesse commented Dec 7, 2023

For the reverse transposition example it would be great to incorporate the GTI_DIRINT function, if anything, at least mention that pvlib offers two reverse transposition algorithms.

I put in a mention and a reference.

@adriesse
Copy link
Member Author

adriesse commented Dec 7, 2023

I guess I'll wait a bit in case more reviews trickle. It ain't over 'til it's over.

It's over!

@adriesse
Copy link
Member Author

adriesse commented Dec 7, 2023

@AdamRJensen Do you think we will have the final form of the paper reference in time for the December release?

I'm a bit puzzled that it is not out yet, but I expect it soon. At the very least we can add the doi: https://doi.org/10.1016/j.solener.2023.112093

In case anyone missed it, the link works now.

@cwhanse cwhanse mentioned this pull request Dec 8, 2023
15 tasks
@adriesse
Copy link
Member Author

My plan is to send a thank-you note to NumFocus after this is merged.

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.

A couple updates needed for the new function name but otherwise LGTM, thanks @adriesse!

My plan is to send a thank-you note to NumFocus after this is merged.

Lovely :) This PR is the last deliverable for the NF grant, right?

docs/sphinx/source/whatsnew/v0.10.3.rst Outdated Show resolved Hide resolved
docs/sphinx/source/reference/irradiance/transposition.rst Outdated Show resolved Hide resolved
@adriesse
Copy link
Member Author

A couple updates needed for the new function name but otherwise LGTM, thanks @adriesse!

My plan is to send a thank-you note to NumFocus after this is merged.

Lovely :) This PR is the last deliverable for the NF grant, right?

That's right, on time and on budget. :)

@kandersolar kandersolar merged commit c18a004 into pvlib:main Dec 12, 2023
30 checks passed
@adriesse
Copy link
Member Author

Thanks @ALL!!

@echedey-ls echedey-ls mentioned this pull request Jun 10, 2024
3 tasks
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.

Demonstrate reverse transposition using Perez-Driesse forward transposition
4 participants