Skip to content

Use offset parameter in equation_of_time_pvcdrom and declination_cooper69 - #2911

Open
chenyi6758-source wants to merge 3 commits into
pvlib:mainfrom
chenyi6758-source:fix/day-angle-offset
Open

chenyi6758-source wants to merge 3 commits into
pvlib:mainfrom
chenyi6758-source:fix/day-angle-offset

Conversation

@chenyi6758-source

Copy link
Copy Markdown
Contributor

Closes #2902.

_calculate_simple_day_angle already takes an offset parameter, so use it directly instead of adding/subtracting the equivalent multiples of 2*pi/365 afterwards. No behavior change: max difference over a full year is 2.4e-14 minutes for equation_of_time_pvcdrom and 7e-16 rad for declination_cooper69.

Verified locally: tests/test_solarposition.py passes (60 passed, 21 skipped).

@mikofski

Copy link
Copy Markdown
Member

Thanks @chenyi6758-source , can you pls add a note & your name to what’s new

@mikofski mikofski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thx!

Comment thread pvlib/solarposition.py
# day angle relative to Vernal Equinox, typically March 22 (day number 81)
bday = \
_calculate_simple_day_angle(dayofyear) - (2.0 * np.pi / 365.0) * 80.0
bday = _calculate_simple_day_angle(dayofyear, offset=81)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is offset=81 and not offset=80 as in the original code?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_calculate_simple_day_angle(dayofyear, offset=1) is the function signature, this is its implementation:
return (2. * np.pi / 365.) * (dayofyear - offset). Both offsets must be added for mathematical equivalence, I've checked that. I haven't checked against the original source.

Added new enhancement details and fixed formatting issues.
@chenyi6758-source

Copy link
Copy Markdown
Contributor Author

Added a whatsnew note under Enhancements in v0.16.2. My name is already in the Contributors section (added with #2869), so no change needed there.

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.

use offset parameter instead of calculating bday in cdrom

4 participants