brandon-rhodes / python-sgp4

Python version of the SGP4 satellite position library
MIT License
377 stars 88 forks source link

`propagate(self, year, month=1, day=1, hour=0, minute=0, second=0.0)` seems kind of wrong and easy to misuse #30

Closed ckuethe closed 4 years ago

ckuethe commented 6 years ago

in particular, one might try to do something like propagate(608400) which might seem reasonable given that the sgp4 function is called assgp4(satrec, seconds_since_epoch) and instead of propagating the satellite a week from the epoch, propagate will try compute for the year 608400.

of course I'd never do something like that... 🤨

Perhaps a better interface for propagate would be propagate(self, obs_time=None, second_since_epoch=None).

If neither obs_time nor seconds_since epoch are given, then propagate would compute at obs_time=datetime.datetime.now() - at the time the function was called. If obs_time is None, and seconds_since_epoch is a real number, then compute based on the epoch of the TLE. If both are given... that should probably be an error because it's not immediately clear what's supposed to happen.

brandon-rhodes commented 5 years ago

(Finally getting time to think about this API issue:) I don't think we can now change this interface since there are already programs that depend on that method's call signature. Would it solve your problem if a second method sitting next to it, with a more informative name, accepted a different way of expressing the time?

As for your other suggestion: I'm not interested, I don't think, in making "now" the default because it makes code unreadable; a new reader too often can't guess what compute() might mean if now isn't explicitly passed it. As I learned long ago after designing another astronomy library. :)

ckuethe commented 5 years ago

Desired end state: make it hard to do the wrong thing. I'm less picky about how exactly we get there.

Maybe another function that takes either a datetime or a timedelta. I'm not sure if it should be clever and guess intent based on whether or not it was given a datetime or a timedelta, or if it should specifically have named args so that users have to be absolutely clear about when they're trying to propagate... "1 week from now, whenever that happens to be" or tell me where it's gonna be at "2019-02-15 00:00:00"

brandon-rhodes commented 5 years ago

Instead of being clever or having multiple arguments, I'd say let's have a method for each possible input. You'll see in Skyfield, for example, if you look at the history of the timelib.py module, that magic arguments have disappeared over time and been replaced with methods that take one kind of argument only, because of problems people — including me! — ran into when there were n ways to pass a date to the same routine, and the routine then had to have if statements inside of it to puzzle out what it was supposed to do.

I'm not sure whether that makes it hard to do the wrong thing — maybe their names could be carefully chosen?

ckuethe commented 5 years ago

In my head _at_time means that "I'm thinking of an absolute point in time at which I want to compute." In my head _from_epoch means that I want to propagate relative to the TLE's epoch, and I might do this every day for satellites whose TLEs are updated frequently (GPS, Iridum, ISS, ...), eg:

propagate_at_time(self, arrow.now().shift(weeks=+3).datetime)

or

propagate_from_epoch(self, datetime.timedelta(weeks=+3).total_seconds())

... Or some other way to get a number of seconds offset from the epoch. I have a pretty good idea of what the magic number 608400 means, but future readers probably would appreciate weeks=+3 rather than 1825200

AlexJohnson1995 commented 5 years ago

@brandon-rhodes , I can't take data from TLE and enter it as a parameter into propagate() function, therefore. But what if I need to propagate to the date mentioned in TLE? Is there any built-in function (maybe, even, not in this library) to convert TLE date to, say, standart time, or, maybe, even the format that can be taken by propagate() function?

brandon-rhodes commented 5 years ago

@AlexJohnson1995 The documentation at https://pypi.org/project/sgp4/ mentions an attribute satellite.epoch which would offer each date component as a separate attribute. If you pass those into propagate, does it correctly compute the satellite's location for that date?

AlexJohnson1995 commented 5 years ago

@brandon-rhodes , It worked, thank you!

brandon-rhodes commented 5 years ago

Wonderful! I'll keep the issue open, though, because it would be nice to have one or two routines added to the API that provided other options.

brandon-rhodes commented 4 years ago

^ (The commit above, alas, named this issue in error.)

@ckuethe — I new releases in the past week that, following your advice here, have deprecated the propagate() routine. I think you are correct that it raises problems, and is not in the spirit of the SGP4 code that inspired this library (and that it now wraps if a C compiler is available!).

So I'll go ahead and close this again, but this time for the real reason: that by taking a pure JD as the input, it's easy for folks to do math on the value, and they can now lean on other libraries to do all kinds of conversions that the values might need.

Let me know if you run into any snags with the new API. Thanks!