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

pvlib.irradiance.poa_horizontal_ratio should be removed #1697

Open
mikofski opened this issue Mar 13, 2023 · 4 comments · May be fixed by #2021
Open

pvlib.irradiance.poa_horizontal_ratio should be removed #1697

mikofski opened this issue Mar 13, 2023 · 4 comments · May be fixed by #2021

Comments

@mikofski
Copy link
Member

Describe the bug
Currently pvlib.irradiance.poa_horizontal_ratio is untested according to codecov

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://app.codecov.io/gh/pvlib/pvlib-python/blob/main/pvlib/irradiance.py#L259

Expected behavior
100% test coverage

Screenshots
image

Versions:

  • pvlib.__version__: 0.9.5-dirty
  • pandas.__version__: ?
  • python: ?

Additional context
This causes codecov to fail for any edits to irradiance.py that reduce the number of lines because then code coverage goes down.

@wholmgren
Copy link
Member

Apparently I added this function 9 years ago during a large refactor, perhaps in an aborted effort at DRY code, but never used it. Tests were hardly a thing at that time. In any case, I suggest we delete it.

@Lakshyadevelops
Copy link
Contributor

@wholmgren Function pvlib.irradiance.poa_horizontal_ratio is being used in example irradiance.ipynb
also what is the following checking for?

try:
    ratio.name = 'poa_ratio'
except AttributeError:
    pass

name attribute of a float?same try-except is for many other functions in irradiance module

@wholmgren
Copy link
Member

Function pvlib.irradiance.poa_horizontal_ratio is being used in example irradiance.ipynb

Those notebooks are not maintained and likely will be deleted in the future.

also what is the following checking for?

If the inputs are series, then ratio is also a Series and this line will assign a name to it. It's not a great pattern.

@kandersolar kandersolar changed the title poa_horizontal_ratio is untested pvlib.irradiance.poa_horizontal_ratio should be removed Apr 5, 2023
@matsuobasho
Copy link
Contributor

matsuobasho commented Nov 10, 2023

Hi, I would like to tackle this issue. It seems like there is more to it aside from just deleting the poa_horizontal_ratio function. If I delete that function, then do I do anything with the docs/tutorials/irradiance.ipynb notebook that uses the function?

Please recommend other requirements for this issue if applicable.

Thanks.

k10blogger added a commit to k10blogger/pvlib-python that referenced this issue Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants