Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adopt ghi_clear variable name for clearsky GHI #2306

Merged
merged 23 commits into from
Dec 16, 2024

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Nov 25, 2024

  • Relates to Standardise variable name for clearsky GHI and DNI, add to glossary #2272
  • I am familiar with the contributing guidelines
  • Tests added and modified
  • 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.

Also related: #1253, #2274

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 25, 2024

Change to haurwitz is non-user facing so the variable name is changed directly (non-breaking change)
Other changes are breaking changes. So far tried adding a test to test_irradiance.py (multiple attempts, here in this pr is the latest version...) but this fails (no warning emitted). Could I get some help with this, please? Not sure if the test is incorrect or the use of renamed_kwarg_warning is incorrect. I am pretty lost here:(

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.

I am pretty lost here:(

Kinda my fault for not documenting what I thought was obvious, sorry 😬
You have to rename the variables in the signature, see suggestions.

RDaxini and others added 2 commits November 25, 2024 16:07
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
RDaxini and others added 2 commits November 26, 2024 07:47
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@RDaxini RDaxini changed the title [WIP] Adopt ghi_clear variable name for clearsky GHI Adopt ghi_clear variable name for clearsky GHI Nov 26, 2024
@RDaxini RDaxini marked this pull request as ready for review November 26, 2024 17:42
RDaxini and others added 3 commits November 26, 2024 15:20
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@RDaxini RDaxini added this to the v0.11.2 milestone Nov 26, 2024
@AdamRJensen
Copy link
Member

I recommend that you hold on working concurrently on this PR (#2306) and #2274 as they concern many of the same functions and will create a headache to merge.

@RDaxini RDaxini marked this pull request as ready for review December 13, 2024 21:34
@RDaxini RDaxini marked this pull request as draft December 13, 2024 21:39
@RDaxini RDaxini marked this pull request as ready for review December 13, 2024 21:41
@RDaxini RDaxini requested a review from kandersolar December 13, 2024 21:41
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.

Ignore the comment below, PR nailed, @RDaxini ! Well done using that random, undocumented decorator.

Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
@RDaxini
Copy link
Contributor Author

RDaxini commented Dec 16, 2024

I think we're good now

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, thanks @RDaxini!

@kandersolar kandersolar merged commit 4462609 into pvlib:main Dec 16, 2024
26 checks passed
@RDaxini RDaxini deleted the ghi_clear_naming branch December 16, 2024 22:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants