snippetpythonMinor
Generate iCalendar .ics files with events for astrological aspects
Viewed 0 times
icalendarastrologicaleventswithgeneratefilesaspectsforics
Problem
I'm relatively new to Python, coming from a deep C++ background. I'm mostly looking for feedback on how to make my code more idiomatic/pythonic, but I would welcome and appreciate any and all other feedback as well.
This code uses the Python library PyEphem, which builds upon XEphem, an X (UNIX GUI) program that's an astrological ephemeris, which is (was historically) a table of celestial bodies and their positions in the sky. PyEphem is used to track the altitudes of astronomical bodies, which are used as an indication of when astrological aspects have or are going to occur.
I'm deploying this code as an AWS Lambda microservice, thus the
Here is the entirety of my module:
```
#!/usr/bin/env python2.7
# aspectus: aspect+prospectus. iCalendars with astrological aspect events.
# Copyright (C) 2017 Frederick Eugene Aumson
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as published
# by the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see .
"""aspectus: aspect+prospectus. Generates iCalendar .ics files to populate your
calendar application with events for astrological aspects (angles) between two
celestial bodies. Currently supports only the trine and sextant aspects, and
only between the sun and the earth. Could easily be extended to support other
bodies, and perhaps with not much more work extended to support aspects between
two bodies outsi
This code uses the Python library PyEphem, which builds upon XEphem, an X (UNIX GUI) program that's an astrological ephemeris, which is (was historically) a table of celestial bodies and their positions in the sky. PyEphem is used to track the altitudes of astronomical bodies, which are used as an indication of when astrological aspects have or are going to occur.
I'm deploying this code as an AWS Lambda microservice, thus the
lambda_handler function. Consider also reviewing the test and deployment Makefile that corresponds to the module below.Here is the entirety of my module:
```
#!/usr/bin/env python2.7
# aspectus: aspect+prospectus. iCalendars with astrological aspect events.
# Copyright (C) 2017 Frederick Eugene Aumson
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as published
# by the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU Affero General Public License for more details.
#
# You should have received a copy of the GNU Affero General Public License
# along with this program. If not, see .
"""aspectus: aspect+prospectus. Generates iCalendar .ics files to populate your
calendar application with events for astrological aspects (angles) between two
celestial bodies. Currently supports only the trine and sextant aspects, and
only between the sun and the earth. Could easily be extended to support other
bodies, and perhaps with not much more work extended to support aspects between
two bodies outsi
Solution
Just reviewing the function
-
The docstring is comprehensive, which is good.
-
But the specification is very complicated, which makes the function hard to use. In particular, the caller has to remember to set
-
In Python, it is conventional to use the names start, stop, and step (in that order) when describing an arithmetic progression: see for example the arguments to
-
When a function needs to return multiple results (as here, where you have an altitude and a datetime) it is usually most convenient to return them as a tuple (rather than a dictionary). That's because the caller can conveniently use tuple unpacking to assign the results to local variables, like this:
If you like having a result with named fields, then you can use a
-
I think
-
In the case where none of the altitudes is crossed, it would be a good idea to raise an exception rather than returning
-
Formatting the docstring into paragraphs would improve its readability. See below for how I would write it (though you might need to mark it up with
-
Here you go through a rather winding path to convert
It would be better to take a shorter path:
But an alternative approach would be to convert all the target altitudes to radians once on input, avoiding the need for further conversions during the search.
-
Here you compare
But if the target altitudes were sorted into numerical order, then you would only need to know the position of
-
Each time around the loop you convert
but it would be simpler to convert the inputs into PyEphem data structures once, and then no conversions would be required in the loop:
-
Once you've found a timestep in which the body has crossed a target altitude, then you can use PyEphem's
```
import ephem
from bisect import bisect
from math import radians
class NotFound(Exception):
pass
def find_altitude(altitudes, body, observer, start, stop, step):
"""Find the first altitude crossed by a body in a time period.
Parameters
altitudes: list(float) -- the target altitudes in degrees
body: Body -- the body
observer: Observer -- observer from whose location the
altitude is calculated.
start: datetime -- start of the period to be searched
stop: datetime -- end of the period to be searched
step: timedelta -- size of the search steps
Returns a pair (altitude, datetime) whose first element is the first
altitude crossed by the body in the time period, and whose second
element is the datetime at which that happens.
If none of the altitudes is crossed in the time period, raise
NotFound.
"""
orig_altitudes = sorted(altitudes)
# Convert inputs to PyEphem data structures.
altitudes = list(map(radians, orig_altitudes))
date = ephem.Date(start)
stop = ephem.Date(stop)
step = step.total_seconds() * ephem.second
def alt(d):
# Return altitude of body as seen by observer at date d.
observer.date = d
body.compute(observer)
ret
find_altitude.- Review
-
The docstring is comprehensive, which is good.
-
But the specification is very complicated, which makes the function hard to use. In particular, the caller has to remember to set
observer.date to the start of the span to be searched, and to call body.compute(observer). It would be easy to forget to do one or both of these, and then the function would go wrong. It would be simpler if find_altitude did both of these tasks. This would require adding an extra parameter giving the start of the period to be searched.-
In Python, it is conventional to use the names start, stop, and step (in that order) when describing an arithmetic progression: see for example the arguments to
range and slice.-
When a function needs to return multiple results (as here, where you have an altitude and a datetime) it is usually most convenient to return them as a tuple (rather than a dictionary). That's because the caller can conveniently use tuple unpacking to assign the results to local variables, like this:
altitude, datetime = find_altitude(...)If you like having a result with named fields, then you can use a
collections.namedtuple, which would give you the best of both worlds.-
I think
altitudes would be a better name than targets.-
In the case where none of the altitudes is crossed, it would be a good idea to raise an exception rather than returning
None. The reason is that it would be easy for the caller to forget to check for the exceptional case, possibly leading to incorrect results, whereas an exception is unambiguous.-
Formatting the docstring into paragraphs would improve its readability. See below for how I would write it (though you might need to mark it up with
:param: and whatnot).-
Here you go through a rather winding path to convert
body.alt to degrees, by converting it to a string representation, parsing that as a float, and then converting that to degrees:altitude = degrees(float(repr(body.alt)))It would be better to take a shorter path:
altitude = degrees(body.alt)But an alternative approach would be to convert all the target altitudes to radians once on input, avoiding the need for further conversions during the search.
-
Here you compare
altitude with each target altitude in turn, taking \$Θ(n)\$ if there are \$n\$ target altitudes:before = [target < altitude for target in targets]But if the target altitudes were sorted into numerical order, then you would only need to know the position of
altitude in the sorted list, which can be found in \$O(\log n)\$ using bisect.bisect. If the position in the sorted list changes, then you know that body has crossed one (or more) of the altitudes.-
Each time around the loop you convert
observer.date to a datetime twice:while True:
# ...
observer.date = observer.date.datetime() + step
if observer.date.datetime() > latest_date:but it would be simpler to convert the inputs into PyEphem data structures once, and then no conversions would be required in the loop:
date = ephem.Date(start)
stop = ephem.Date(stop)
step = step.total_seconds() * ephem.second
while True:
# ...
date += step
if date > stop:
raise NotFound()
observer.date = date-
Once you've found a timestep in which the body has crossed a target altitude, then you can use PyEphem's
newton function to efficiently find the exact crossing point. This uses the Newton-Raphson method to get quadratic convergence (the number of correct digits roughly doubles in each step), whereas the the approach in the post only has linear convergence.- Revised code
```
import ephem
from bisect import bisect
from math import radians
class NotFound(Exception):
pass
def find_altitude(altitudes, body, observer, start, stop, step):
"""Find the first altitude crossed by a body in a time period.
Parameters
altitudes: list(float) -- the target altitudes in degrees
body: Body -- the body
observer: Observer -- observer from whose location the
altitude is calculated.
start: datetime -- start of the period to be searched
stop: datetime -- end of the period to be searched
step: timedelta -- size of the search steps
Returns a pair (altitude, datetime) whose first element is the first
altitude crossed by the body in the time period, and whose second
element is the datetime at which that happens.
If none of the altitudes is crossed in the time period, raise
NotFound.
"""
orig_altitudes = sorted(altitudes)
# Convert inputs to PyEphem data structures.
altitudes = list(map(radians, orig_altitudes))
date = ephem.Date(start)
stop = ephem.Date(stop)
step = step.total_seconds() * ephem.second
def alt(d):
# Return altitude of body as seen by observer at date d.
observer.date = d
body.compute(observer)
ret
Code Snippets
altitude, datetime = find_altitude(...)altitude = degrees(float(repr(body.alt)))altitude = degrees(body.alt)before = [target < altitude for target in targets]while True:
# ...
observer.date = observer.date.datetime() + step
if observer.date.datetime() > latest_date:Context
StackExchange Code Review Q#153958, answer score: 4
Revisions (0)
No revisions yet.