Skip to content

sunrise and sunset from pyephem #588

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 35 commits into from
Oct 4, 2018
Merged

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Sep 21, 2018

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

  • Addresses issue add sunrise / sunset to Location #114
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

@@ -453,6 +471,63 @@ def _ephem_setup(latitude, longitude, altitude, pressure, temperature):
return obs, sun


def ephem_next_rise_set(time, latitude, longitude, altitude=0,
Copy link
Member

Choose a reason for hiding this comment

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

would next_sun_rise_set_pyephem be more consistent with your preferred naming scheme?

Copy link
Member Author

@cwhanse cwhanse Sep 21, 2018

Choose a reason for hiding this comment

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

Yes, I'll change. I have in mind Location.get_next_sunrise, Location.get_previous_sunrise as wrapper methods.

result_rounded = pd.DataFrame(index=result.index)
# need to iterate because to_datetime does not accept 2D data
# the rounding fails on pandas < 0.17
for col, data in result.iteritems():
result_rounded[col] = (pd.to_datetime(
np.floor(data.values.astype(np.int64) / 1e9)*1e9, utc=True)
np.floor(data.values.astype(np.int64) / 60e9)*60e9, utc=True)
Copy link

Choose a reason for hiding this comment

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

E226 missing whitespace around arithmetic operator

result_rounded = pd.DataFrame(index=result.index)
for col, data in result.iteritems():
result_rounded[col] = (pd.to_datetime(
np.floor(data.values.astype(np.int64) / 60e9)*60e9, utc=True)
Copy link

Choose a reason for hiding this comment

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

E226 missing whitespace around arithmetic operator

]).tz_localize('MST').tolist()
transit = pd.DatetimeIndex([datetime.datetime(2015, 1, 2, 12, 5, 0),
datetime.datetime(2015, 8, 2, 12, 7, 0)
]).tz_localize('MST').tolist()
Copy link

Choose a reason for hiding this comment

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

E124 closing bracket does not match visual indentation

result_rounded = pd.DataFrame(index=result.index)
for col, data in result.iteritems():
result_rounded[col] = data.dt.round('min').tz_convert('MST')
del expected_rise_set('transit')
Copy link

Choose a reason for hiding this comment

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

E999 SyntaxError: can't delete function call

@cwhanse
Copy link
Member Author

cwhanse commented Oct 1, 2018

Sigh. PyEphem has the same behavior as spa. If, on a given day D, sunset occurs after midnight UTC of the next day D+1 (common in summer in northern hemisphere), pyephem's next_setting function will return D's sunset, not D+1 sunset, when passed D+1 even when localized. Why? PyEphem converts all dates to it's own integer system (days since 1899/12/31 12:00:00 UTC) without adjusting for the timezone offset, then finds the next sunset after the requested datetime.

@cwhanse
Copy link
Member Author

cwhanse commented Oct 3, 2018

Fixed the timezone behavior. Ready for review/approve/merge.

@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@pvlib pvlib deleted a comment from stickler-ci bot Oct 3, 2018
@wholmgren
Copy link
Member

Looks good to me. You can resolve the merge conflict locally or in the web interface (click resolve conflicts and then delete the >>> --- <<< cruft). Usually github will automatically take the union of this file. Not sure why it's not working this time.

@wholmgren wholmgren added this to the 0.6.1 milestone Oct 4, 2018
@wholmgren
Copy link
Member

I went ahead and resolved the conflict. Merging...

@wholmgren wholmgren merged commit 0c245a9 into pvlib:master Oct 4, 2018
@mikofski mikofski mentioned this pull request Oct 5, 2018
8 tasks
@cwhanse cwhanse deleted the ephem_rise_set branch February 4, 2019 16:50
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.

3 participants