skyfielders / python-skyfield

Elegant astronomy for Python
MIT License
1.4k stars 211 forks source link

searchlib.find_discrete issue:'function' object has no attribute 'rough_period' #715

Closed davidmikolas closed 2 years ago

davidmikolas commented 2 years ago

From https://rhodesmill.org/skyfield/searches.html it looks like the function we define for searchlib.find_discrete() returns a boolean and has no unusual attribute requirements.

But when I run the script below I get

Traceback (most recent call last):
  File "crescent_moon_v00.py", line 24, in <module>
    q = searchlib.find_discrete(ts.ut1_jd(jd), ts.ut1_jd(jd+0.25), moon_is_4_degrees)
  File "/Users/davido/anaconda3/lib/python3.7/site-packages/skyfield/searchlib.py", line 30, in find_discrete
periods = (jd1 - jd0) / f.rough_period
AttributeError: 'function' object has no attribute 'rough_period'

Have I made a dumb mistake or should I create a 'rough_period' attribute myself?

note: I have not done an existence test, there might not be (and there wasn't in this case) a True/False transition in the time range specified. I've assumed Skyfield would throw an exception like "no event found". Choosing a time range where there is a True/False transition doesn't help.

from skyfield.api import Loader, Topos
import numpy as np
import matplotlib.pyplot as plt
from skyfield import searchlib

load = Loader('~/Documents/fishing/SkyData') # avoid multiple copies of large files
ts = load.timescale()
eph = load('de421.bsp')

earth, sun, moon = [eph[x] for x in ('earth', 'sun', 'moon')]

Monument_Hill = Topos('33.38 N', '112.31 W') # https://youtu.be/yYg68Bwc2VI

def moon_is_4_degrees(time):
    moon_astrometric = (earth + Monument_Hill).at(time).observe(moon)
    moon_alt, az, d = moon_astrometric.apparent().altaz()
    return moon_alt.degrees < 4

print('test: moon_is_4_degrees(ts.now()): ', moon_is_4_degrees(ts.now()))

jds = np.arange(3) + 2459581.5

for jd in jds:
    q = searchlib.find_discrete(ts.ut1_jd(jd), ts.ut1_jd(jd+0.25), moon_is_4_degrees)
    print(jd, q)
brandon-rhodes commented 2 years ago

Have I made a dumb mistake or should I create a 'rough_period' attribute myself?

You should provide a step_days attribute yourself — if you double-check the documentation, you should find a mention of it. The rule is: Skyfield checks for step_days, then if it's not there, it checks for rough_period because long ago that used to be the attribute Skyfield looked for and there might still be users whose code provides it. But that makes the error message fairly useless, doesn't it? Let's keep this issue open until I've updated the exception to ask for the right attribute!

davidmikolas commented 2 years ago

I see, yes it's nicely covered provided one reads all of the text and doesn't just grab the mars_quadrature() example and run with it.

There must have been some thinking about why make step_days a user-added attribute instead of a parameter of find_discrete() the way the begin and end time parameters are.

That way there would be some evidence of it in help(find_discrete) so we wouldn't have to connect to the internet and navigate to the documentation page in order to know of the requirement.

Anyway my problem seems solved by adding the following. Thank you for the speedy response, and as always, Skyfield rocks!

moon_is_4_degrees.step_days = 0.05
Screen Shot 2022-03-03 at 12 43 30 PM
brandon-rhodes commented 2 years ago

There — the two commits you'll see above both improve the documentation example, so it can be safely cut-and-pasted (I myself was confused on re-reading until I saw the attribute was set later on!), and the exception now names the correct attribute. These improvements should appear at the next release of Skyfield.

Skyfield rocks!

Thanks for the encouraging words!

davidmikolas commented 2 years ago

Excellent; thank you!