Skip to content

Fixed inconsistent azimuth angle in the ephemeris function #41

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

Merged
merged 8 commits into from
Mar 26, 2015

Conversation

uvchik
Copy link
Contributor

@uvchik uvchik commented Mar 25, 2015

Fixed #40.

uvchik added 7 commits March 25, 2015 09:57

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…n module

reverted version of RA (Feb 18,2014)

Partially verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
@uvchik
Copy link
Contributor Author

uvchik commented Mar 25, 2015

The results of ephemeris are now similar to the results of pyephem. Both results are feasible for the location "Berlin/Europe" but I didn't checked the details.

@wholmgren wholmgren added the bug label Mar 25, 2015
@wholmgren
Copy link
Member

Thanks. Did you change anything about the algorithm itself (other than south=180) or is everything just PEP8 changes? It's hard to see on the diff.

Are you interested in getting into the details, improving the function, and thoroughly testing it, or would you prefer to just go ahead and call it good? While I'm all for the careful approach on the principle of making a good and dependable library, I'm not sure if it's worth the time. Also, @alorenzo175 was working on implementing NREL's SPA algorithm with numba wrappers.

Pascals.
temperature : float
temperature : float [default: 12]
Copy link
Member

Choose a reason for hiding this comment

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

Most of the pvlib docstrings do not have defaults in the parameter descriptions. This is mostly because I personally don't like to put them there, and I removed them when cleaning up the docstrings. I think I might be in the minority, though, so maybe consider starting an issue for this. I'm ok with keeping this here, as long as we know about the inconsistencies.

@wholmgren
Copy link
Member

Can you go ahead and run the modified code on the standard NREL test? See test_spa_physical and test_pyephem_physical. Then we'll at least have something concrete to say about the accuracy.

@uvchik
Copy link
Contributor Author

uvchik commented Mar 25, 2015

I made different commits to make it easier to understand.

  1. uvchik/pvlib-python@9b95720: This is the bug fix. I just reverted the changes from RA.
  2. uvchik/pvlib-python@bbb8b4e: This is the test function. That means it works now.
  3. I removed the default values from the docstring because I like consistence more than I like default values.
  4. uvchik/pvlib-python@e991712: This commit contains all the pep8 changes, except the ones that are right next to one of the other changes (e.g. within the docstring). @wholmgren The changes above are just pep8. Trailing spaces were remove by my IDE automatically.

@uvchik
Copy link
Contributor Author

uvchik commented Mar 25, 2015

Physical feasibility test.

DaFrOut_a = pvlib.solarposition.get_solarposition(time=data.index,
            location=location, method='pyephem')

DaFrOut_b = pvlib.solarposition.get_solarposition(time=data.index,
            location=location, method='ephemeris')

# zenith over azimuth
plt.plot(DaFrOut_a['azimuth'], 90 - DaFrOut_a['zenith'], '.')
plt.plot(DaFrOut_b['azimuth'], 90 - DaFrOut_b['zenith'], '.')   
plt.show()

# zenith over index(time)
plt.plot(90 - DaFrOut_a['zenith'], '-')
plt.plot(90 - DaFrOut_b['zenith'], '-')   
plt.show()

In the first plot both models look pretty much the same but in the second plot you can see an offset of one hour between the methods (two hours during the summer period).

Half an hour of this is corrected in line 312 (there is already o comment of @wholmgren )

# ToDo: Will H: surprised to see the 0.5 here, but moving on...
UnivHr = DecHours + utc_offset  #- .5

If there would be a minus 1 than everything would be the same between the two models during the winter time. But I cannot explain it so I didn't changed it.

  1. We have an offset of one hour.
  2. We have different behaviour considering the summer/winter time.

Any ideas?

I'm absent until Monday.

@uvchik
Copy link
Contributor Author

uvchik commented Mar 25, 2015

Changed the offset from line 312 to "-1":

The plots from the code above:

Plot of zenith over azimuth (blue = 'pyephem', green='ephemeris')
solarposition
Plot of zenith over index(time) (blue = 'pyephem', green='ephemeris') [winter-summer-time]
winter-summer-time

@wholmgren wholmgren added this to the 0.1 milestone Mar 25, 2015
@wholmgren
Copy link
Member

Ok, I just wanted to make sure that there were no other changes hidden in the PEP8 commit.

I'm not sure where exactly the problem is. My memory is that I never got the results that I expected from this function, didn't trust it, and after making minimal effort at compatibility with the Location class, I gave up on it in favor of making a pyephem wrapper. I thought that the utc_offset code handled daylight savings time, but apparently not (no DST is one of the few things that Arizona does right!). After that, @Calama-Consulting added the NREL spa and nobody seriously looked at ephemeris again until now.

I'm ok with merging this now and opening a new PR for addressing the remaining problems in 0.2. Maybe keep #40 open instead of making a new issue. Sound good @Calama-Consulting or @bmu?

@uvchik
Copy link
Contributor Author

uvchik commented Mar 26, 2015

The difference between the two sun rises of the green line is 23 hours. Summer time means that the time is set one hour ahead. At two o'clock the time is set to three o'clock. So the ephemeris keeps the real local time and the pyephem keeps the local standard time (winter time). I would prefer the second variant but I have to look how our weather data set is like.

Still don't understand the -1 in line 312.

@wholmgren I agree with merging it and moving the time shift problem to a new issue. I can do that next week.

@jforbess
Copy link
Contributor

I strongly agree that processing modules should keep to standard (winter)
time, unless we have thought through a lot of hard problems that apparently
python and pandas are still working through themselves regarding the
ambiguity of daylight savings.

On Thu, Mar 26, 2015 at 3:28 AM, ukrien [email protected] wrote:

The difference between the two sun rises of the green line is 23 hours.
Summer time means that the time is set one hour ahead. At two o'clock the
time is set to three o'clock. So the ephemeris keeps the real local time
and the pyephem keeps the local standard time (winter time). I would prefer
the second variant but I have to look how our weather data set is like.

Still don't understand the -1 in line 312.

@wholmgren https://github.com/wholmgren I agree with merging it and
moving the time shift problem to a new issue. I can do that next week.


Reply to this email directly or view it on GitHub
#41 (comment).

@wholmgren
Copy link
Member

I'm going to go ahead and merge and we'll deal with the tz problem separately.

wholmgren added a commit that referenced this pull request Mar 26, 2015
Fixed inconsistent azimuth angle in the ephemeris function
@wholmgren wholmgren merged commit b3a091f into pvlib:master Mar 26, 2015
@wholmgren wholmgren mentioned this pull request Mar 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

solarposition.ephemeris
3 participants